Bug 201077 – [GTK][WPE] webkit_settings_set_user_agent() allows content forbidden in HTTP headers

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Adrian Perez]

The following call will succeed, and result in invalid characters
being sent over the wire:

   webkit_settings_set_user_agent(settings, "\x1B");  /* Escape character */

As a reminder: the HTTP specification indicates that headers must
contain only visible, printable characters (so control characters
like 0x1B are forbidden).

If an user-agent string with an embeeded newline character is set,
libsoup will actually warn:

   soup_message_headers_append: assertion 'strpbrk (value, "\r\n") == NULL' failed

I am not sure what libsoup does (maybe it ignores setting the header?),
but at any rathe an HTTP header must *not* contain newlines embedded in
their values.

It would be good to have some validation to ensure that WebKitSettings
never ends up with an user-agent string configured which would result
in incorrect requests being sent to servers. The particular case of
control characters is something I have observed after an use-after-free
ended up setting some garbage string as user-agent, and some servers
would start mysteriously returning HTTP 400 (that is: “Bad Request”).

[Adrian Perez]

In a chat with Claudio Saavedra about this we agreed that
we could consider that passing a string as user agent which
would result in an incorrect HTTP header can be considered a
programming error. That way, we can validate the string and
return early using g_return_if_fail() —which will warn— thus
avoiding the need to change the API/ABI.

[Adrian Perez]

Created attachment 377168 [details]
Patch

[EWS Watchlist]

Attachment 377168 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3076:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3078:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adrian Perez]

Maybe I should also add some new API function to check whether
a string can be used as User-Agent string. Something like:

   gboolean webkit_is_valid_user_agent (const gchar *user_agent);

WDYT? It would be useful to validate them before trying to call
webkit_settings_set_user_agent().

[Adrian Perez]

(In reply to Build Bot from comment #3)
> Attachment 377168 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3076:  One line
> control clauses should not use braces.  [whitespace/braces] [4]
> ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3078:  One line
> control clauses should not use braces.  [whitespace/braces] [4]
> Total errors found: 2 in 2 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

These are false positives: notice the CHECK_OK macro, which expands

   parseFoo(CHECK_OK);

into:

   parseFoo(ok);
   if (!ok)
       return;
   ((void) 0);

Therefore the braces are needed because those are not really one-line
clauses. This preprocessor trick is very handy to make sure that the
“ok” status flag is always passed to the parsing functions and checked
afterwards.

[Carlos Garcia Campos]

Comment on attachment 377168 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377168&action=review

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3037
> +// Checks User-Agent strings as per https://tools.ietf.org/html/rfc7231#section-5.5.3
> +// The comments with the BNF productions before the functions below have been copied
> +// from the RFC verbatim as reference.
> +struct UserAgentChecker {

I think this belongs to Source/WebCore/platform/network/HTTPParsers.cpp You can probably reuse code from there.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #6)
> Comment on attachment 377168 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377168&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3037
> > +// Checks User-Agent strings as per https://tools.ietf.org/html/rfc7231#section-5.5.3
> > +// The comments with the BNF productions before the functions below have been copied
> > +// from the RFC verbatim as reference.
> > +struct UserAgentChecker {
> 
> I think this belongs to Source/WebCore/platform/network/HTTPParsers.cpp You
> can probably reuse code from there.

Oh, cool, somehow my grepping didn't unearth this file. I see there are
other utility functions already there like isValidLanguageHeaderValue()
already, so I will move the code to this file, make the entry point be
isValidUserAgentHeaverValue() and make the code style similar to the
rest of the file.

Thanks for the pointer!

[Adrian Perez]

Created attachment 377247 [details]
WIP Patch

Work in progress moving things to HTTPParsers.cpp; I will 
be adding some unit tests in Tools/TestWebKitAPI/Tests/WebCore/HTTPParsers.cpp
for the new code.

[EWS Watchlist]

Attachment 377247 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adrian Perez]

Created attachment 377707 [details]
Patch

[Adrian Perez]

Created attachment 377713 [details]
Patch v2


This adds the symbol to the linker script, which should make the
mac and iOS EWS bots happy.

[Adrian Perez]

Created attachment 377718 [details]
Patch v3


Well, I was wrong: it seems that the right thing to do is marking the
symbol using WEBCORE_EXPORT, rather than adding it to WebCore.order.

Third time is the charm. Maybe.

[Adrian Perez]

Ping reviewers :-)

[Carlos Garcia Campos]

Comment on attachment 377718 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=377718&action=review

LGTM

> Source/WebCore/platform/network/HTTPParsers.cpp:177
> +// See RFC 7230, Section 3.2.6.
> +static inline bool isOctectInFieldContentCharacter(const UChar c)
> +{
> +    // obs-text = %x80-FF
> +    return (c >= 0x80 && c <= 0xFF);
> +}
> +
> +// See RFC 7230, Section 3.2.6.
> +static bool isCommentTextCharacter(const UChar c)
> +{
> +    // ctext = HTAB / SP
> +    //       / %x21-27 ; '!'-'''
> +    //       / %x2A-5B ; '*'-'['
> +    //       / %x5D-7E ; ']'-'~'
> +    //       / obs-text
> +    return (c == '\t' || c == ' '
> +        || (c >= 0x21 && c <= 0x27)
> +        || (c >= 0x2A && c <= 0x5B)
> +        || (c >= 0x5D && c <= 0x7E)
> +        || isOctectInFieldContentCharacter(c));
> +}

We have these already in HTTPHeaderField.cpp maybe we can move them to a common place?

> Source/WebCore/platform/network/HTTPParsers.cpp:246
> +static inline bool skipCharacter(const String& value, unsigned& pos, const UChar expected)
> +{
> +    if (pos < value.length() && value[pos] == expected) {
> +        ++pos;
> +        return true;
> +    }
> +    return false;
> +}

Can't this be used by passing a predicate doing the value[pos] == expected check?

> Source/WebCore/platform/network/HTTPParsers.cpp:275
> +// True is a comment is present, incrementing "pos" to the position after the comment.

True if

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3048
> +    g_return_if_fail(WebCore::isValidUserAgentHeaderValue(userAgentString));

I think we should only validate the given user agent. The standard user agent is generated by WebKit, it's expected to be valid. We could add an assert in WebCore::standardUserAgent() before returnig the generated value, though.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #14)
> Comment on attachment 377718 [details]
> Patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377718&action=review
> 
> LGTM

\o/

> > Source/WebCore/platform/network/HTTPParsers.cpp:177
> > +// See RFC 7230, Section 3.2.6.
> > +static inline bool isOctectInFieldContentCharacter(const UChar c)
> > +{
> > +    // obs-text = %x80-FF
> > +    return (c >= 0x80 && c <= 0xFF);
> > +}
> > +
> > +// See RFC 7230, Section 3.2.6.
> > +static bool isCommentTextCharacter(const UChar c)
> > +{
> > +    // ctext = HTAB / SP
> > +    //       / %x21-27 ; '!'-'''
> > +    //       / %x2A-5B ; '*'-'['
> > +    //       / %x5D-7E ; ']'-'~'
> > +    //       / obs-text
> > +    return (c == '\t' || c == ' '
> > +        || (c >= 0x21 && c <= 0x27)
> > +        || (c >= 0x2A && c <= 0x5B)
> > +        || (c >= 0x5D && c <= 0x7E)
> > +        || isOctectInFieldContentCharacter(c));
> > +}
> 
> We have these already in HTTPHeaderField.cpp maybe we can move them to a
> common place?

There is already a couple of utility function prototypes declared
in “HTTPHeaderField.h” inside the WebCore::RFC7230 namespace. We could
add the prototypes for the other functions used by “HTTPParsers.cpp”
and use them from there.

How about doing that in a follow-up patch?

> > Source/WebCore/platform/network/HTTPParsers.cpp:246
> > +static inline bool skipCharacter(const String& value, unsigned& pos, const UChar expected)
> > +{
> > +    if (pos < value.length() && value[pos] == expected) {
> > +        ++pos;
> > +        return true;
> > +    }
> > +    return false;
> > +}
> 
> Can't this be used by passing a predicate doing the value[pos] == expected
> check?

True, let's change it to that!

(Probably I wrote have written the version that takes just the
character before writing the overload that takes a predicate,
then forgot or didn't notice that one could be written in terms
of the other.)

> > Source/WebCore/platform/network/HTTPParsers.cpp:275
> > +// True is a comment is present, incrementing "pos" to the position after the comment.
> 
> True if
> 
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3048
> > +    g_return_if_fail(WebCore::isValidUserAgentHeaderValue(userAgentString));
> 
> I think we should only validate the given user agent. The standard user
> agent is generated by WebKit, it's expected to be valid. We could add an
> assert in WebCore::standardUserAgent() before returnig the generated value,
> though.

Done.

[Adrian Perez]

Created attachment 378579 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 378579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378579&action=review

Clearing r flag since this is already reviewed

> Source/WebCore/platform/network/HTTPParsers.h:75
> +WEBCORE_EXPORT bool isValidUserAgentHeaderValue(const String&);

We don't need the export since this is only used by linux ports

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3053
> +        userAgentString = WebCore::standardUserAgent("");

I would add an assert in WebCore::standardUserAgent() to ensure the generated user agent is valid.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #18)
> Comment on attachment 378579 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378579&action=review
> 
> Clearing r flag since this is already reviewed
> 
> > Source/WebCore/platform/network/HTTPParsers.h:75
> > +WEBCORE_EXPORT bool isValidUserAgentHeaderValue(const String&);
> 
> We don't need the export since this is only used by linux ports

...but we need the export, otherwise TestWebCore does not link.
Or do you mean that the unit tests added by the patch should be
built only for the GTK and WPE ports, too?

> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3053
> > +        userAgentString = WebCore::standardUserAgent("");
> 
> I would add an assert in WebCore::standardUserAgent() to ensure the
> generated user agent is valid.

Ah, I missed this, so I'll add it before landing.

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #19)
> (In reply to Carlos Garcia Campos from comment #18)
> > Comment on attachment 378579 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=378579&action=review
> > 
> > Clearing r flag since this is already reviewed
> > 
> > > Source/WebCore/platform/network/HTTPParsers.h:75
> > > +WEBCORE_EXPORT bool isValidUserAgentHeaderValue(const String&);
> > 
> > We don't need the export since this is only used by linux ports
> 
> ...but we need the export, otherwise TestWebCore does not link.
> Or do you mean that the unit tests added by the patch should be
> built only for the GTK and WPE ports, too?

No no, you are right, I forgot the unit test, sorry.

> > > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3053
> > > +        userAgentString = WebCore::standardUserAgent("");
> > 
> > I would add an assert in WebCore::standardUserAgent() to ensure the
> > generated user agent is valid.
> 
> Ah, I missed this, so I'll add it before landing.

:-)

[Adrian Perez]

Created attachment 378644 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 378644 [details]
Patch for landing

Clearing flags on attachment: 378644

Committed r249810: <https://trac.webkit.org/changeset/249810>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adrian Perez]

(In reply to Adrian Perez from comment #15)
> (In reply to Carlos Garcia Campos from comment #14)
> > Comment on attachment 377718 [details]
> >
> > > Source/WebCore/platform/network/HTTPParsers.cpp:177
> > > +// See RFC 7230, Section 3.2.6.
> > > +static inline bool isOctectInFieldContentCharacter(const UChar c)
> > > +{
> > > +    // obs-text = %x80-FF
> > > +    return (c >= 0x80 && c <= 0xFF);
> > > +}
> > > +
> > > +// See RFC 7230, Section 3.2.6.
> > > +static bool isCommentTextCharacter(const UChar c)
> > > +{
> > > +    // ctext = HTAB / SP
> > > +    //       / %x21-27 ; '!'-'''
> > > +    //       / %x2A-5B ; '*'-'['
> > > +    //       / %x5D-7E ; ']'-'~'
> > > +    //       / obs-text
> > > +    return (c == '\t' || c == ' '
> > > +        || (c >= 0x21 && c <= 0x27)
> > > +        || (c >= 0x2A && c <= 0x5B)
> > > +        || (c >= 0x5D && c <= 0x7E)
> > > +        || isOctectInFieldContentCharacter(c));
> > > +}
> > 
> > We have these already in HTTPHeaderField.cpp maybe we can move them to a
> > common place?
> 
> There is already a couple of utility function prototypes declared
> in “HTTPHeaderField.h” inside the WebCore::RFC7230 namespace. We could
> add the prototypes for the other functions used by “HTTPParsers.cpp”
> and use them from there.
> 
> How about doing that in a follow-up patch?

Filed bug #201721 for the follow-up refactoring, I'll give it a go in
the next days.