377052 - nsBaseURLParser::ParseURL doesn't handle spaces embedded in the spec proprely

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Brett Wilson]

97     for (p = spec; len && *p && !colon && !slash; ++p, --len) {
 98         // skip leading whitespace and control characters
 99         if (*p > '\0' && *p <= ' ') {
100             spec++;
101             specLen--;
102             continue;
103         }
            ...

When nsBaseURLParser::ParseURL finds a space in the input, it shifts the input over by one byte. This correctly handles leading spaces on URL specs. However, this case still runs if there are spaces in the middle of the spec, or trailing spaces. In these other cases, it still shifts the start of the string over, corrupting the spec.

This could conceivably be a security problem because by adding spaces to the middle of a URL, I can cause the parser to ignore characters at the beginning of the string, potentially making the domain name parse to something else. If the spec is given to another component that parses the URL differently, they will see different domain names, opening cross-site scripting attacks.

This should not be a security problem in practice because all code because everybody uses this same parser and the results are canonicalized after that, which should permanently remove any of the characters at the beginning that were skipped.

Comment 1

[Patrick McManus [:mcmanus]]

valentin - what do you think the current state of this is?

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Attachment: nsBaseURLParser::ParseURL doesn't handle spaces embedded in the scheme properly

The comment is correct. That loop actually only runs for the scheme. And since a scheme can't be matched if it contains spaces, we never actually hit this corner case. But I think it's worth fixing.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e60a3ad8b3948dc40e04ac9e77a8dadc34b6717
Bug 377052 - nsBaseURLParser::ParseURL doesn't handle spaces embedded in the scheme properly r=mcmanus

Comment 4

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4e60a3ad8b39