Investigate and correct overly permissive parsing in //net due to base::StringToInt() |
|||||||||||||
Issue descriptionbase::StringToInt() is being used in places that expect decimal number strings. These callers typically want to recognize strings like "345" and "-891". The problem is StringToInt() accepts other strings like "+34". Depending on the context, accepting numbers with a leading plus is unexpected and wrong. This is particularly a problem for protocol parsing code in //net where strictness is important for adhering to standards, so the added convenience in what StringToInt() accepts is not desirable. A recent example of this is issue 596258 . But I have also seen this come up in HTTP header parsing reviews for instance (i.e. parsing the HTTP version number). The default of StringToInt() is not great for //net code, and may be contributing bugs elsewhere. Doing a quick grep through //net shows there are other callers using it which may be problematic. Possible remedies are: (a) Make the policy of permitting a leading + an explicit parameter to base::StringToInt(), to make it obvious to future callers. (b) Introduce a //net specific wrapper that does stricter parsing (c) Fix each callsite on a one-off basis ⛆ |
|
|
,
Mar 21 2016
,
Mar 21 2016
This applies to all of the string-to-int functions. For instance: * StringToInt() * StringToUint() * StringToInt64() * StringToUint64() * StringToSizeT()
,
Mar 21 2016
,
Mar 21 2016
,
Mar 21 2016
,
Mar 21 2016
Having reviewed a number of the current bugs, my proposal is to create helper functions in //net/base for doing simple decimal number parsing. As the majority of callers are wanting to just parse: 1*DIGIT (Don't even need negatives, even though using an "int"). I think the versions in base:: are subtle, but there are enough callers throughout the rest of the codebase that introducing a required parameter at this point will be a big undertaking. There are also callers (like in WebSockets) that avoid this pitfall of StringToInt() by creating their own wrappers. All of that code could similarly be switched to the generic wrapper.
,
Mar 21 2016
I agree with your assessment, though you might want to loop in base/ owners for their take. It's possible this is causing subtle bugs elsewhere.
,
Mar 21 2016
+thakis for feedback on comment #7 from the perspective of a //base owner. An alternative to //net specific helpers would be to add a required parameter in //base to help catch this problem elsewhere in the codebase. Not having reviewed all the other callers of base::StringToInt(), my concern would be most callsites outside of //net don't care about strictness and just want to parse a number (for which this new API would just be kludgy).
,
Mar 23 2016
I will proceed with a //net specific solution. Based on that experience, we can consider generalizing to //base as a follow-up.
,
Mar 23 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/94e42f599fa98662868c993cc4e432fd25a40f99 commit 94e42f599fa98662868c993cc4e432fd25a40f99 Author: eroman <eroman@chromium.org> Date: Wed Mar 23 22:07:08 2016 Add base/parse_number.h to generalize number parsing done in //net. In particular, the current use of base::StringToInt() is problematic as it permits strings with a leading plus sign, which is invalid in most places where //net parses numbers. BUG=596523, 596538 Review URL: https://codereview.chromium.org/1829873002 Cr-Commit-Position: refs/heads/master@{#382936} [modify] https://crrev.com/94e42f599fa98662868c993cc4e432fd25a40f99/net/base/host_port_pair.cc [modify] https://crrev.com/94e42f599fa98662868c993cc4e432fd25a40f99/net/base/host_port_pair_unittest.cc [modify] https://crrev.com/94e42f599fa98662868c993cc4e432fd25a40f99/net/base/ip_address.cc [add] https://crrev.com/94e42f599fa98662868c993cc4e432fd25a40f99/net/base/parse_number.cc [add] https://crrev.com/94e42f599fa98662868c993cc4e432fd25a40f99/net/base/parse_number.h [add] https://crrev.com/94e42f599fa98662868c993cc4e432fd25a40f99/net/base/parse_number_unittest.cc [modify] https://crrev.com/94e42f599fa98662868c993cc4e432fd25a40f99/net/base/sdch_manager.cc [modify] https://crrev.com/94e42f599fa98662868c993cc4e432fd25a40f99/net/net.gypi
,
Mar 23 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1f90fcb579588d6491f04ffc586ba2bf5b2a1653 commit 1f90fcb579588d6491f04ffc586ba2bf5b2a1653 Author: eroman <eroman@chromium.org> Date: Wed Mar 23 22:28:55 2016 Don't allow negative date components in ParseCertificateDate(), or ones starting with a plus. BUG=596523, 596544 Review URL: https://codereview.chromium.org/1832583002 Cr-Commit-Position: refs/heads/master@{#382949} [modify] https://crrev.com/1f90fcb579588d6491f04ffc586ba2bf5b2a1653/net/cert/x509_cert_types.cc [modify] https://crrev.com/1f90fcb579588d6491f04ffc586ba2bf5b2a1653/net/cert/x509_cert_types_unittest.cc
,
Mar 24 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/55fa65c5ce1c15d8a492d6876b2324fd576a83a4 commit 55fa65c5ce1c15d8a492d6876b2324fd576a83a4 Author: eroman <eroman@chromium.org> Date: Thu Mar 24 18:57:49 2016 Don't allow a leading plus in ports for proxy bypass rules. For instance will reject the rule "*.org:+443" whereas before it would be accepted. BUG=596523, 596570 Review URL: https://codereview.chromium.org/1831653002 Cr-Commit-Position: refs/heads/master@{#383111} [modify] https://crrev.com/55fa65c5ce1c15d8a492d6876b2324fd576a83a4/net/proxy/proxy_bypass_rules.cc [modify] https://crrev.com/55fa65c5ce1c15d8a492d6876b2324fd576a83a4/net/proxy/proxy_bypass_rules_unittest.cc
,
Apr 11 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae40d9bae35935b815cb651b2a79d32ee78707fa commit ae40d9bae35935b815cb651b2a79d32ee78707fa Author: eroman <eroman@chromium.org> Date: Mon Apr 11 18:40:26 2016 Extend net/base/parse_number.h for parsing of negative numbers, and determining if there was overflow/underflow. BUG=596523 Review URL: https://codereview.chromium.org/1828103002 Cr-Commit-Position: refs/heads/master@{#386425} [modify] https://crrev.com/ae40d9bae35935b815cb651b2a79d32ee78707fa/net/base/host_port_pair.cc [modify] https://crrev.com/ae40d9bae35935b815cb651b2a79d32ee78707fa/net/base/ip_address.cc [modify] https://crrev.com/ae40d9bae35935b815cb651b2a79d32ee78707fa/net/base/parse_number.cc [modify] https://crrev.com/ae40d9bae35935b815cb651b2a79d32ee78707fa/net/base/parse_number.h [modify] https://crrev.com/ae40d9bae35935b815cb651b2a79d32ee78707fa/net/base/parse_number_unittest.cc [modify] https://crrev.com/ae40d9bae35935b815cb651b2a79d32ee78707fa/net/base/sdch_manager.cc [modify] https://crrev.com/ae40d9bae35935b815cb651b2a79d32ee78707fa/net/cert/x509_cert_types.cc [modify] https://crrev.com/ae40d9bae35935b815cb651b2a79d32ee78707fa/net/proxy/proxy_bypass_rules.cc
,
Apr 11 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2a545c6ef2899f1218bfd06ee6ff9666a8ff304f commit 2a545c6ef2899f1218bfd06ee6ff9666a8ff304f Author: eroman <eroman@chromium.org> Date: Mon Apr 11 20:59:07 2016 Fix number parsing problems with HttpResponseHeaders::GetAgeValue() to not accept invalid numbers. BUG=596523, 596554 Review URL: https://codereview.chromium.org/1827243002 Cr-Commit-Position: refs/heads/master@{#386467} [modify] https://crrev.com/2a545c6ef2899f1218bfd06ee6ff9666a8ff304f/net/http/http_response_headers.cc [modify] https://crrev.com/2a545c6ef2899f1218bfd06ee6ff9666a8ff304f/net/http/http_response_headers.h [modify] https://crrev.com/2a545c6ef2899f1218bfd06ee6ff9666a8ff304f/net/http/http_response_headers_unittest.cc
,
Apr 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8a43d8aa92e2bdb795fbccd67a3e8add8d3a9e8 commit e8a43d8aa92e2bdb795fbccd67a3e8add8d3a9e8 Author: eroman <eroman@chromium.org> Date: Tue Apr 12 06:02:06 2016 Fix number parsing for max-age for HSTS/HPKP. * Don't allow leading '+' * Dont' allow trailing garbage (for overflowed numbers) BUG=596523, 596561 Review URL: https://codereview.chromium.org/1831963002 Cr-Commit-Position: refs/heads/master@{#386596} [modify] https://crrev.com/e8a43d8aa92e2bdb795fbccd67a3e8add8d3a9e8/net/http/http_security_headers.cc [modify] https://crrev.com/e8a43d8aa92e2bdb795fbccd67a3e8add8d3a9e8/net/http/http_security_headers.h [modify] https://crrev.com/e8a43d8aa92e2bdb795fbccd67a3e8add8d3a9e8/net/http/http_security_headers_unittest.cc [modify] https://crrev.com/e8a43d8aa92e2bdb795fbccd67a3e8add8d3a9e8/net/http/transport_security_state.h
,
Apr 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/38ab932422f88d294a7869ed512c41d6bfec3236 commit 38ab932422f88d294a7869ed512c41d6bfec3236 Author: eroman <eroman@chromium.org> Date: Tue Apr 12 19:38:56 2016 Fix parsing of max-age in SdchManager. BUG= 596541 , 596523 Review URL: https://codereview.chromium.org/1871383005 Cr-Commit-Position: refs/heads/master@{#386766} [modify] https://crrev.com/38ab932422f88d294a7869ed512c41d6bfec3236/net/base/sdch_manager.cc [modify] https://crrev.com/38ab932422f88d294a7869ed512c41d6bfec3236/net/base/sdch_manager_unittest.cc
,
Aug 1
,
Aug 1
,
Aug 2
|
||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by eroman@chromium.org
, Mar 21 2016