Bug 89750

Created attachment 148980 [details] WIP.

Comment on attachment 148980 [details] WIP. View in context: https://bugs.webkit.org/attachment.cgi?id=148980&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:184 > + return path.startsWith(m_path, false); There are some open questions that should be discussed on public-webappsec@ before this lands, even as an experiment. 1. This implementation is strictly string-matching: it doesn't do any fancy directory parsing (or even non-fancy splitting on `/`). Is that enough? 2. Currently, paths behave as though they had a wildcard at the end. That is: `/path` matches `/path/to/file.js`, but also `/pathsareawesome/file.js`. I'm tempted to say that this is good enough, since ending the rule with a `/` would remove ambiguity (e.g. `/path/`), but it's something that should be discussed. 3. Query strings and anchors are part of the path: should they be? > LayoutTests/http/tests/security/contentSecurityPolicy/1.1/source-list-parsing-paths-01.html:12 > + ['no', 'script-src 127.0.0.1:*/sec/', 'resources/script.js'], This shows the wildcard-like behavior noted above. I think it makes sense, but I'd like some other opinions. :)

Comment on attachment 148980 [details] WIP. View in context: https://bugs.webkit.org/attachment.cgi?id=148980&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:511 > - m_list.append(CSPSource(m_origin->protocol(), m_origin->host(), m_origin->port(), false, false)); > + m_list.append(CSPSource(m_origin->protocol(), m_origin->host(), m_origin->port(), "", false, false)); You can use String() here, which is more efficient because it doesn't cause a malloc.

> 1. This implementation is strictly string-matching: it doesn't do any fancy directory parsing (or even non-fancy splitting on `/`). Is that enough? One thing to consider is URL canonicalization. For example, what happens if the source-list or the URL itself contains a %-encoded character? We probably want to treat "equivalent" URLs equivalently. > 2. Currently, paths behave as though they had a wildcard at the end. That is: `/path` matches `/path/to/file.js`, but also `/pathsareawesome/file.js`. I'm tempted to say that this is good enough, since ending the rule with a `/` would remove ambiguity (e.g. `/path/`), but it's something that should be discussed. Hum... I wonder if we should add an implicit / if there isn't one in the policy. That seems least surprising. > 3. Query strings and anchors are part of the path: should they be? That doesn't quite seem right. I wonder if we should ignore everything after a ? or a # in the source-list, just like we previously ignored everything after a /. > > LayoutTests/http/tests/security/contentSecurityPolicy/1.1/source-list-parsing-paths-01.html:12 > > + ['no', 'script-src 127.0.0.1:*/sec/', 'resources/script.js'], > > This shows the wildcard-like behavior noted above. I think it makes sense, but I'd like some other opinions. :) That's an interesting idea, but probably not something there's a strong need to support.

We should probably move this discussion to the public-webappsec list. But, since we're here: (In reply to comment #4) > > 1. This implementation is strictly string-matching: it doesn't do any fancy directory parsing (or even non-fancy splitting on `/`). Is that enough? > > One thing to consider is URL canonicalization. For example, what happens if the source-list or the URL itself contains a %-encoded character? We probably want to treat "equivalent" URLs equivalently. You're right. I didn't even think about that. > > 2. Currently, paths behave as though they had a wildcard at the end. That is: `/path` matches `/path/to/file.js`, but also `/pathsareawesome/file.js`. I'm tempted to say that this is good enough, since ending the rule with a `/` would remove ambiguity (e.g. `/path/`), but it's something that should be discussed. > > Hum... I wonder if we should add an implicit / if there isn't one in the policy. That seems least surprising. How would a user specify a file named "path" if we added an implicit `/`? Do we want users to be able to specify individual files? I think we probably do. > > 3. Query strings and anchors are part of the path: should they be? > > That doesn't quite seem right. I wonder if we should ignore everything after a ? or a # in the source-list, just like we previously ignored everything after a /. I'm with you on anchors. Let's throw those out. I'm wondering if there's value in query strings, however. Maybe there's javascript that returns different results based on a user ID that's passed in as a GET param? Perhaps a developer could use CSP to ensure that only the most recent version of her javascript loads if a version is encoded in a GET param? Who knows what crazy things programmers do? :)

Questions 2 and 3 are related. If we let folks specify individual files, then it makes sense to let them specify query parameters as well.

Ok. If we're going to allow explicit files, then we might want to use a * to indicate a wild card. Similar to what we do with host names, we should probably only allow the * at the end and immediately after a / character.

(In reply to comment #7) > Ok. If we're going to allow explicit files, then we might want to use a * to indicate a wild card. Similar to what we do with host names, we should probably only allow the * at the end and immediately after a / character. I've been thinking about this a bit, and I've reconsidered. I don't think file-level whitelisting is critical, and it adds significant complexity to the processing (for instance, what do we do if a developer names a file `*`? :) ). I'd now suggest that the first pass we make at this feature keep the granularity at the directory level. We can always add the complexity of individual file-level sources later, and developers have the simple workaround of putting that one file they care about in a directory that's whitelisted. With that in mind, I'd like to upload a patch that would implicitly end the path with a `/` if one isn't already present, and throw a warning if the path contains either `?` or `#`. I think that has the best potential to be clearly understood, and meets at least 80% of the use case. WDYT?

Ok. Sounds good.

Created attachment 150613 [details] Slightly less WIP.

Created attachment 150615 [details] Slightly less WIP.

Comment on attachment 150615 [details] Slightly less WIP. View in context: https://bugs.webkit.org/attachment.cgi?id=150615&action=review I think this addresses the core of your feedback, Adam. Would you mind taking another look? I'd appreciate suggestions on the path parsing bit; it seems less efficient than it could be. > Source/WebCore/page/ContentSecurityPolicy.cpp:501 > + path = KURL(m_scriptExecutionContext->url(), String(begin, end - begin)).path(); This seems like an expensive way of parsing the path, but it's the best I could find (that didn't involve replicating huge swaths of code). Are there better mechanisms for canonicalizing paths?

Comment on attachment 150615 [details] Slightly less WIP. Attachment 150615 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13125518

Comment on attachment 150615 [details] Slightly less WIP. Attachment 150615 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13128503 New failing tests: http/tests/security/contentSecurityPolicy/source-list-parsing-06.html http/tests/security/contentSecurityPolicy/source-list-parsing-05.html

Created attachment 150624 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 150615 [details] Slightly less WIP. Attachment 150615 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13129527

Comment on attachment 150615 [details] Slightly less WIP. View in context: https://bugs.webkit.org/attachment.cgi?id=150615&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:188 > + return path.startsWith(m_path, false); This test is overly simplistic. You need to do some sort of comparison that ignores encoding differences. > Source/WebCore/page/ContentSecurityPolicy.cpp:230 > + String m_directiveName; > + ScriptExecutionContext* m_scriptExecutionContext; I'd prefer not to store this state on CSPSourceList. Perhaps the source list can report its error to some higher level object that has a back pointer to ScriptExecutionContext? > Source/WebCore/page/ContentSecurityPolicy.cpp:486 > + while (position < end) { > + skipWhile<isPathComponentCharacter>(position, end); four-space indent pls. > Source/WebCore/page/ContentSecurityPolicy.cpp:496 > + logInvalidPath(String(begin, end - begin)); Let's worry about logging errors in an later patch. This line is pushing us to have this object keep a bunch of state that it doesn't really need or want. >> Source/WebCore/page/ContentSecurityPolicy.cpp:501 >> + path = KURL(m_scriptExecutionContext->url(), String(begin, end - begin)).path(); > > This seems like an expensive way of parsing the path, but it's the best I could find (that didn't involve replicating huge swaths of code). Are there better mechanisms for canonicalizing paths? I'm not sure what you're trying to accomplish here. We should just store String(begin, end - begin) into path and use a comparison function that understands %-escaping. It's not the case that KURL canonicalizes paths anyway, so this isn't really saving us any work.

Thanks, Adam! I'll fiddle with this more if you can point me in the right direction (see below). (In reply to comment #17) > (From update of attachment 150615 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150615&action=review > > > Source/WebCore/page/ContentSecurityPolicy.cpp:188 > > + return path.startsWith(m_path, false); > > This test is overly simplistic. You need to do some sort of comparison that ignores encoding differences. Ideally, I think it would be this simplistic here. It seems like it would be best to do any necessary processing once at parse time, rather than every time we check the URL. > > Source/WebCore/page/ContentSecurityPolicy.cpp:486 > > + while (position < end) { > > + skipWhile<isPathComponentCharacter>(position, end); > > four-space indent pls. Arg. I need to figure out how to make vim treat files inside my webkit repository strangely. :) > > Source/WebCore/page/ContentSecurityPolicy.cpp:496 > > + logInvalidPath(String(begin, end - begin)); > > Let's worry about logging errors in an later patch. This line is pushing us to have this object keep a bunch of state that it doesn't really need or want. Agreed, I'll rip this out. > > >> Source/WebCore/page/ContentSecurityPolicy.cpp:501 > >> + path = KURL(m_scriptExecutionContext->url(), String(begin, end - begin)).path(); > > > > This seems like an expensive way of parsing the path, but it's the best I could find (that didn't involve replicating huge swaths of code). Are there better mechanisms for canonicalizing paths? > > I'm not sure what you're trying to accomplish here. We should just store String(begin, end - begin) into path and use a comparison function that understands %-escaping. It's not the case that KURL canonicalizes paths anyway, so this isn't really saving us any work. KURL does some work on the path that I'd like to avoid replicating (processing `..` for instance: I'm looking specifically at KURL::parse, around http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/KURL.cpp&type=cs&l=1360). I don't know the code that well, however, so it's very possible I've misinterpreted what it's doing there. Do you have a specific comparison function in mind? I'm creating a KURL here because I'm given a KURL in all the `*Matches()` functions, and I thought it best to do the work here once rather than repeating any processing in the test, but I'm certainly happy to reuse any components I've overlooked.

I wonder if we should use the same algorithm that's used for cookies: http://tools.ietf.org/html/rfc6265#section-5.1.4 Specifically, cookie punt on all this canonicalization stuff.

(In reply to comment #19) > I wonder if we should use the same algorithm that's used for cookies: > > http://tools.ietf.org/html/rfc6265#section-5.1.4 > > Specifically, cookie punt on all this canonicalization stuff. That seems like a reasonable place to start. We can always add complexity in later.

Moving the test-skipping out to a separate patch.

*** Bug 89288 has been marked as a duplicate of this bug. ***

Created attachment 159210 [details] Patch

Picking this back up after a month or so. The patch is a bit smaller now, and I think I've found a good solution for URL-decoding paths before comparison: 'decodeURLEscapeSequences' seems to be exactly what I was looking for back in June. Would you mind taking a look, Adam? If this is an implementation that still makes sense to you, I'll take a stab at speccing it out. Thanks! -mike

Comment on attachment 159210 [details] Patch Attachment 159210 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13530165

Created attachment 160599 [details] Patch

Created attachment 160601 [details] Fixing Mac.

Comment on attachment 160601 [details] Fixing Mac. Attachment 160601 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13594807 New failing tests: http/tests/cache/post-with-cached-subresources.php

Created attachment 160603 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

(In reply to comment #28) > (From update of attachment 160601 [details]) > Attachment 160601 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/13594807 > > New failing tests: > http/tests/cache/post-with-cached-subresources.php Hrm. I'm fairly confident that this isn't a failure generated by this patch, and it passes here on my mac locally. I'll try it on my linux box at the office tomorrow, but I suspect it's just flake.

Yeah http/tests/cache/post-with-cached-subresources.php is very flaky. We need to mark it as such.

Comment on attachment 160601 [details] Fixing Mac. View in context: https://bugs.webkit.org/attachment.cgi?id=160601&action=review > Source/WebCore/ChangeLog:10 > + `http://example.com/path/to/a/file/javascript.js`, but also from `````````````````` > Source/WebCore/page/ContentSecurityPolicy.cpp:518 > + skipWhile<isPathComponentCharacter>(position, end); > + > + // path/to/file.js > + // ^ > + if (position < end && skipExactly(position, end, '/')) > + continue; Why not add '/' to isPathComponentCharacter?

This looks great. My only concern here is how to deal with CSP_NEXT. Previously, my thought was that we would expose CSP 1.0 in the Content-Security-Policy header and CSP 1.1 features in X-WebKit-CSP. However, we wouldn't be able to do that the way you've structured the ifdefs.

Let me ask on the webappsec call tomorrow what the other vendors want to do here.

(In reply to comment #33) > This looks great. My only concern here is how to deal with CSP_NEXT. Previously, my thought was that we would expose CSP 1.0 in the Content-Security-Policy header and CSP 1.1 features in X-WebKit-CSP. However, we wouldn't be able to do that the way you've structured the ifdefs. I agree with you that it'd be best to expose 1.0 through the unprefixed header. If we put a "prefixed" boolean on CSPSourceListDirective, one option would be to choose between implementation of both ignorePath() and parsePath(), and ifdef the implementation of the latter if CSP_NEXT isn't enabled. Basically, whatever you decide on the call, we can make work. :) That said, I'd _much_ rather get the unprefixed header out the door than nail down the 1.1 path processing detail. (In reply to comment #32) > (From update of attachment 160601 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160601&action=review > > > Source/WebCore/ChangeLog:10 > > + `http://example.com/path/to/a/file/javascript.js`, but also from > > `````````````````` ''''''''''''''''' I was so careful! > > > Source/WebCore/page/ContentSecurityPolicy.cpp:518 > > + skipWhile<isPathComponentCharacter>(position, end); > > + > > + // path/to/file.js > > + // ^ > > + if (position < end && skipExactly(position, end, '/')) > > + continue; > > Why not add '/' to isPathComponentCharacter? I think I originally thought we'd want to do some extra validation here, flagging 'path//to/whatever', but it's probably not necessary.

I asked on the webappsec telecon today. They seemed agreeable to keeping the 1.1 features on the X-WebKit-CSP header. I'm going to send a follow email to give folks some time to think about it.

(In reply to comment #36) > I asked on the webappsec telecon today. They seemed agreeable to keeping the 1.1 features on the X-WebKit-CSP header. I'm going to send a follow email to give folks some time to think about it. Great. While that discussion's open, would you like me to take a stab at updating the relevant bits of the spec in line with this patch?

Please!

(In reply to comment #38) > Please! Added in https://dvcs.w3.org/hg/content-security-policy/rev/abb64ba225c4 and kicked off a very quiet (so far) discussion at http://lists.w3.org/Archives/Public/public-webappsec/2012Sep/0001.html. This implementation somewhat depends on how we decide to proceed with the 1.0/1.1 headers. There hasn't been a ton of discussion on that thread... Now that we're more formally working towards moving to to CR, perhaps you could poke folks on the next call? :)

IMHO, we should use the approach outlined in http://lists.w3.org/Archives/Public/public-webappsec/2012Aug/0007.html Specifically, we should keep all the CSP 1.1 specific behavior in the prefixed header *except* path restrictions, which we should implement in the Content-Security-Policy header. The email messages outlines why, but the gist is that it's an easy position to walk away from by loosening the semantics later.

(In reply to comment #40) > IMHO, we should use the approach outlined in http://lists.w3.org/Archives/Public/public-webappsec/2012Aug/0007.html > > Specifically, we should keep all the CSP 1.1 specific behavior in the prefixed header *except* path restrictions, which we should implement in the Content-Security-Policy header. The email messages outlines why, but the gist is that it's an easy position to walk away from by loosening the semantics later. I think that makes sense. If we go that route, would we also want to change the behavior for ports that haven't opted-in to CSP_NEXT? That would be a fairly trivial change to this patch, and I think it's the direction we want to go, but I'm not sure to what extent we can bring other ports with us. More broadly, once 1.0 hits CR and 1.1 moves to some sort of official draft status (I assume?), do we need to maintain the CSP_NEXT flag?

> If we go that route, would we also want to change the behavior for ports that haven't opted-in to CSP_NEXT? Probably. > That would be a fairly trivial change to this patch, and I think it's the direction we want to go, but I'm not sure to what extent we can bring other ports with us. IMHO, we should make sure the Content-Security-Policy header works the same in every port. > More broadly, once 1.0 hits CR and 1.1 moves to some sort of official draft status (I assume?), do we need to maintain the CSP_NEXT flag? We can ask other folks for input, but my guess is that once CSP 1.1 hits FPWD, we can drop the CSP_NEXT flag and just keep the 1.1 directives in the X-WebKit-CSP header.

Created attachment 164573 [details] Patch

Created attachment 164575 [details] No backtics. :(

Hey Adam, I think this implements what we've discussed. Paths are supported outside the CSP_NEXT flag, which means they'll be available for all ports, both on the current prefixed header and the upcoming canonical header. Also, the changelog is completely devoid of the cursed '`'. As it turns out, it's not at all dependent on the canonical header patch that's also in-flight. It still probably makes sense to land them together in order to minimize the amount of flux we impose on ports, but if you'd like to land this now and the other later, I wouldn't complain. :) WDYT?

Comment on attachment 164575 [details] No backtics. :( Looks great. Let's wait for after the M23 branch before landing.

Comment on attachment 164575 [details] No backtics. :( (In reply to comment #46) > (From update of attachment 164575 [details]) > Looks great. Let's wait for after the M23 branch before landing. http://trac.webkit.org/browser/branches/chromium/1271 has been cut, so I think this should be safe to land. CQ?

Comment on attachment 164575 [details] No backtics. :( Clearing flags on attachment: 164575 Committed r129143: <http://trac.webkit.org/changeset/129143>

All reviewed patches have been landed. Closing bug.