Closed Bug 377052 Opened 13 years ago Closed 4 years ago

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


(Core :: Networking, defect, critical)

Not set



Tracking Status
firefox47 --- fixed


(Reporter: brettw, Assigned: valentin)


(Whiteboard: [necko-backlog])


(1 file)

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.
valentin - what do you think the current state of this is?
Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-backlog]
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.
Attachment #8714290 - Flags: review?(mcmanus)
Assignee: nobody → valentin.gosu
Attachment #8714290 - Flags: review?(mcmanus) → review+
Bug 377052 - nsBaseURLParser::ParseURL doesn't handle spaces embedded in the scheme properly r=mcmanus
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Flags: needinfo?(valentin.gosu)
You need to log in before you can comment on or make changes to this bug.