Bug 99274

Summary:

Warn when CSP headers don't separate directives with ';'.

Product:

WebKit

Reporter:

Mike West <mkwst>

Component:

WebCore Misc.

Assignee:

Mike West <mkwst>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, webkit.review.bot

Priority:

Keywords:

WebExposed

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

93197

Attachments:

Description	Flags
Patch	none
Patch	none

Mike West

Reported 2012-10-14 13:56:24 PDT

If a web author specifies "default-src 'self' img-src *", they're almost certainly making a mistake. We might want to throw a warning if we see a source list that contains a directive name. See https://twitter.com/soaj1664ashar/status/257468299372277760 for example. WDYT, Adam?

Attachments
Patch (15.00 KB, patch) 2012-10-15 01:26 PDT, Mike West	no flags	Details Formatted Diff Diff
Patch (10.74 KB, patch) 2012-10-15 12:37 PDT, Mike West	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Adam Barth

Comment 1 2012-10-14 23:00:17 PDT

Hum... Maybe if there's a source-expression that matches a directive name, we should issue a warning? Technically it could be an intranet host with that name, but that's pretty unlikely.

Mike West

Comment 2 2012-10-15 01:26:20 PDT

Created attachment 168647 [details] Patch

Mike West

Comment 3 2012-10-15 01:27:57 PDT

(In reply to comment #1) > Hum... Maybe if there's a source-expression that matches a directive name, we should issue a warning? Technically it could be an intranet host with that name, but that's pretty unlikely. That's what I was thinking as well. The attached patch is a stab at that. It also moves the directive names out of CSPDirectiveList::addDirective into static methods of ContentSecurityPolicy so I can reuse them. This seems to be the right thing to do, but I'm not entirely sure. I saw a few 'static char xxx[] = "...";' lines while grepping around... WDYT?

Adam Barth

Comment 4 2012-10-15 09:19:03 PDT

Comment on attachment 168647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168647&action=review > Source/WebCore/page/ContentSecurityPolicy.h:87 > + static const char* connectSrc() { return "connect-src"; } > + static const char* defaultSrc() { return "default-src"; } > + static const char* fontSrc() { return "font-src"; } > + static const char* frameSrc() { return "frame-src"; } > + static const char* imgSrc() { return "img-src"; } > + static const char* mediaSrc() { return "media-src"; } > + static const char* objectSrc() { return "object-src"; } > + static const char* reportURI() { return "report-uri"; } > + static const char* sandbox() { return "sandbox"; } > + static const char* scriptSrc() { return "script-src"; } > + static const char* styleSrc() { return "style-src"; } > +#if ENABLE(CSP_NEXT) > + static const char* formAction() { return "form-action"; } > + static const char* pluginTypes() { return "plugin-types"; } > + static const char* scriptNonce() { return "script-nonce"; } > +#endif Rather thank making these functions, we should just make them static constants in the cpp file. > Source/WebCore/page/ContentSecurityPolicy.h:89 > + static bool isDirectiveName(const String& name); Should this be private?

Mike West

Comment 5 2012-10-15 12:37:34 PDT

Created attachment 168755 [details] Patch

Mike West

Comment 6 2012-10-15 22:07:42 PDT

Cool. Throwing this into the queue.

WebKit Review Bot

Comment 7 2012-10-15 22:25:37 PDT

Comment on attachment 168755 [details] Patch Clearing flags on attachment: 168755 Committed r131413: <http://trac.webkit.org/changeset/131413>

WebKit Review Bot

Comment 8 2012-10-15 22:25:41 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.