Semicolons are accepted in attribute values but they can't be set

[macintorsten]

When setting a cookie the API accepts an attribute value that contains a semicolon, and it does not give any error. If the attribute value is coming from an intrusted source it can lead to injection of new attributes than the intended one.

Example: path = '/numbers;domain=.iana.org'
Cookies('nameofcookie', 'cookievalue', {path: '/numbers;domain=.iana.org'})

Reproduce;

location='https://www.iana.org/protocols'
$.getScript('https://cdn.jsdelivr.net/npm/js-cookie@2/src/js.cookie.min.js')
Cookies('nameofcookie', 'cookievalue', {path: '/numbers;domain=.iana.org'})
console.log(document.cookie)

Output: [EMPTY]

location='/numbers'
console.log(document.cookie)

Output: nameofcookie=cookievalue

I think the path value cannot contain a semicolon but I am not completely sure.

[FagnerMartinsBrack]

js-cookie is just an adapter to document.cookie API. If the document.cookie API accepts a semicolon in the Path, js-cookie will just read from that. Any suggestions of what can be done in js-cookie side?

Sorry, I misread your issue. I'll give it more thought later. In the meantime, I would advise not to allow untrusted sources to set the cookie, even its attributes. That can lead to a README change.

[FagnerMartinsBrack]

Ok, cool, here's the long version.

I've tested and confirmed this locally, here are my thoughts:

There's no such API

There's no such API as Cookies(String, String, Object Literal). Please, refer to the docs for supported usage and don't use undocumented APIs just because the internals allows you to. Here's a relevant content on the subject.

The problem is in the application code, not js-cookie

The problem happens when the developer allows an untrusted source to set the cookie attributes.

The following example can't be exploited if the developer pass an untrusted input in the cookie value due to the default encoding mechanism and the fact RFC 6265 doesn't allow ; in the cookie value:

Cookies.set('nameofcookie', 'cookievalue;path=/path')

In this case, the cookie is escaped. The browser creates it as document.cookie = "nameofcookie=cookievalue%3Bpath=/path".

However, the following example can be exploited if the developer uses a raw converter:

const RawWriteCookies = Cookies.withConverter({ write: (rawValue) => rawValue });
RawWriteCookies.set('nameofcookie', 'cookievalue;path=/path')

In this case, the cookie is created as document.cookie = "nameofcookie=cookievalue;path=/path"

Besides the cookie value, the path attribute is the only vulnerable to this. In this case, because it's impossible for a website to set a cookie to another domain that is not its own, it can only exploit subdomains.

The following is also vulnerable to XSS:

Cookies.set('nameofcookie', 'cookievalue', { path: '/;domain=sub.domain.com' })

js-cookie will set as document.cookie = "nameofcookie=cookievalue;path=/;domain=sub.domain.com"

Since js-cookie is just an adapter on document.cookie it's not advisable to rely on untrusted input for any of its arguments, for that it can expose the application into an XSS attack. Not just for the value of the cookie (which is pretty obvious) but also to its attributes.

@macintorsten Can you think of real cases where this can be exploited?

Conclusion

This is not a problem with js-cookie. It's a problem with an application code that sets the path of the cookie using an untrusted input. However, because this behavior is astonishing, we might be able to do something to make this issue more obvious, or explicitly sanitize the attributes that are set through the API.

The README has an example that mentions the use of path: window.location.pathname. That's not a good practice and is there just to show a problem with IE. We can improve the docs to remove that:

@macintorsten Do you have any suggestions on which approach to take for this?
Ping @carhartl.

This issue is probably present since forever, and since this is the first time somebody is reporting, I wonder if it's a big deal.

[FagnerMartinsBrack]

If you really allow untrusted input to your cookies, the attacker can do some funny things such as:

Cookies.set('nameofcookie', 'cookievalue', { path: '/', domain: '; secure' })

Again, I'm not sure how practical is that and I guess the assumption is that people shouldn't be sending untrusted input for the same reason as:

document.cookie = "nameofcookie=cookievalue; Path=/" + untrustedInput;

[FagnerMartinsBrack]

This issue is probably present since forever

Hmm, it isn't. It used explicit attribute positioning up to version 2.1.4. After that, it started appending the attributes to the cookie String and that caused the issue.

The commit has been in the codebase for a year.

[FagnerMartinsBrack]

Sorry for the back-and-forth, but yeah it is present for a long time 😅. The problem is the same if we explicitly position the value on the attributes:

return (document.cookie = [
  key, '=', value,
  attributes.expires ? '; expires=' + attributes.expires.toUTCString() : '', // use expires attribute, max-age is not supported by IE
  // attributes.path ? '; path=' + attributes.path : '',
  attributes.path ? '; path=' + '/;domain=sub.domain.com' : '',
  attributes.domain ? '; domain=' + attributes.domain : '',
  attributes.secure ? '; secure' : ''
].join(''));

See v2.1.3.

[FagnerMartinsBrack]

Looking for ideas on how to fix this, if that's at all possible, without removing or unnecessarily escaping the ; from the attributes:

QUnit.only('sanitization of attributes to prevent XSS from untrusted input', function (assert) {
	assert.expect(1);
        var expectedResult = 'c=v; path=/;domain=sub.domain.com';
	assert.strictEqual(Cookies.set('c', 'v', {
		path: '/;domain=sub.domain.com'
	}), expectedResult, 'should not set domain from withing the path attribute');
});

@macintorsten what's the expectedResult in this case?

@Badger Could you lend a hand to this one? It's not clear from the RFC 6265 that there are any limitations to what Path accepts as the attribute-value/cookie-path. Am I missing something?

[macintorsten]

Hi, I am not personally affected by this issue and I don't know enough about RFC 6265 (or the interpretation of it) to give a definite answer how it should be handled. I stumbled upon this behavior when I tried to find a way to set a path containing a ;, something that seems impossible. This behavior might not have a huge impact but I can think of a theoretical example where it can be exploited.

Let's assume you have a webapp on www.site.com/appname/. Because the application can be installed on any path (eg. /appname/ or /something/else/) the developer might not be able to make assumptions about the URL on which the application resides, therefore he/she will set the cookie as in the README example. Let's further assume this code is present a login page directly on http://www.site.com/appname/:

Cookies.set('CSRF-TOKEN', '123randomstring123', { path: location.pathname });

(If CSRF value is available to js-code it might not be needed to also put in a cookie, but it can be a convenient place to store it perhaps). The cookie will store an anti CSRF token whose value will be sent in a request (as post/get parameter) to the server to protect against CSRF attacks, for all important state-changing requests.

If an attacker directs a victim to http://www.site.com/appname;domain=.site.com/, it can given special circumstanses give the same page back as http://www.site.com/appname/ because everything after a ; in a path is not considered to be part of the path as seen by the (server side part) application according to many web servers (i think these are called path parameters). This URL will make the victim's browser store a cookie scoped for all subdomains to site.com. If the attacker then has an XSS (or controls a DNS entry) on a subdomain to site.com (not matter what path) it is possible for the attacker to steal the CSRF-token and perform a CSRF attack after the user has logged in.

This is a made up example, but I guess there should be more examples like this that can be made up.

[macintorsten]

I think the goal for the API should be that when reading back an attribute-value (if it can be done) or when viewing it in browser's developer tool, the value should correspond to the same value as the API was fed with for the attribute of the same name. The value provided should be the value that is in effect in the browser.

According to the RFC 6265 it's mentioned that path (not limited to just path-value) can't contain a a semi-colon:

path-value        = <any CHAR except CTLs or ";">

Already here we see it's impossible to fulfill the goal because the API can be called with a value containing a semi-colon which is not a valid value according to RFC:

Link: https://tools.ietf.org/html/rfc6265#section-4.1

And in section 5.2 describes how a client should parse a set-cookie header:

The user agent MUST use an algorithm equivalent to the following algorithm to parse the unparsed-attributes [...]

In particular step 3 further down, describing how to parse unparsed-attributes is interesting:

3.  If the remaining unparsed-attributes contains a %x3B (";") character:

    Consume the characters of the unparsed-attributes up to, but not including, the first %x3B (";") character.

Otherwise

    Consume the remainder of the unparsed-attributes.

Let the `cookie-av` string be the characters consumed in this step.

This means cookie-av (path-av path-key-value-pair is one type cookie-av) cannot contain semi-colon and in practice it will be "truncated" at the first occurance of a semi-colon (or actually the next attribute key/value set will begin and parse if valid or discarded or not valid).

I think that if you call Cookies.set('name', 'value', { path: 'abc;def' }) either there should be an error/exception or the value should be truncated to just abc... or maybe the invalid attribute should just be silently ignored.

But this is just my point of view.

[FagnerMartinsBrack]

This means cookie-av (path-av path-key-value-pair is one type cookie-av) cannot contain semi-colon and in practice it will be "truncated" at the first occurance of a semi-colon (or actually the next attribute key/value set will begin and parse if valid or discarded or not valid).

I think that if you call Cookies.set('name', 'value', { path: 'abc;def' }) either there should be an error/exception or the value should be truncated to just abc... or maybe the invalid attribute should just be silently ignored.

But this is just my point of view.

Good catch!

I think we should respect RFC 6265 and truncate in the first ;. Here's the test:

QUnit.only('sanitization of attributes to prevent XSS from untrusted input', function (assert) {
	assert.expect(1);
        var expectedResult = 'c=v; path=/';
	assert.strictEqual(Cookies.set('c', 'v', {
		path: '/;domain=sub.domain.com'
	}), expectedResult, 'should not set domain from withing the path attribute');
});

Do you want to create a PR for that since you're the one bringing this up? I'll probably schedule a patch version.

@carhartl If you have some time, can you please read through and make sure this is the correct approach to take? There should be enough information in this thread to understand the issue.

[carhartl]

I am slightly leaning towards throwing an exception if that path attribute contained an invalid ;. This seems to be slightly more informative for users unaware of this sublety, instead of silently fixing the input.

What about the other attributes? Would it be possible to inject something malicious using the domain attribute and somehow manipulate the path? I can't quite come up with another made up attack scenario.

[FagnerMartinsBrack]

By default, js-cookie has never thrown any exception or logged any message in the console. Similar things have been suggested on #86 and #88.

We just have to be aware that any message we write in the code will increase the "~900 bytes GZip" size, which is one of its advertised features as stated in the README.

For a couple of years there's been a plan to implement a js.cookie.debug.js or something that would be used for debugging and would contain a lot of informative messages.

There's only one report of this problem with the Path attribute, but given this is a potential attack vector should we make an exception and include it in the source code without waiting for the js.cookie.debug.js?

Or we can add to the README docs, then we wouldn't need more characters in the code.

[carhartl]

document.cookie = ... should be suffering from the same attack vector. So maybe that isn’t something we should worry about.

So ignoring that for a while, I’m wondering how document.cookie = ... treats RFC 6265 non compliant input. Maybe it does the silent truncating for us anyway in modern browsers these days. E.g. document.cookie = "foo=bar; path=/baz;xyz" will be equivalent to document.cookie = "foo=bar; path=/baz" anyway, in which case I would suggest to not do anything about it (and save even more precious bytes).

[FagnerMartinsBrack]

When I get some time, I'll investigate the possibilities and see how many bytes this may add.

Maybe because the problem is in an attribute and the class is encapsulated, it is expected from the developer that js-cookie doesn't have this kind of problems. The interim solution can be to document the problem in the README and remove the example that shows the window.location being set to the path.

[macintorsten]

I'm not sure I know how this should be fixed.

I think there is small difference between using the js-cookie api and calling document.cookie directly. The latter accepts a string similar to http response header set-cookie and it will ignore anything it does not recognize (which is likely the case after a ; which is not a valid value), and you provide the whole set-cookie string value at once. If you are using document.cookie to set a cookie with certain attributes it's the developer's job to parse the input and construct the set-cookie string according the correct syntax.

In the js-cookie API however you can set just the path attribute value solely, as an independent attribute value. A developer that uses the api to set path is likely to make the following two assumptions when setting a cookie with a certain path value. 1) The value provided will be the effective value used for attribute 'path' for the cookie 2) The value will only be set for the 'path' attribute and not be able to affect any other attribute, regardless of value passed to it. The developer does not necessarily know how the API is implemented.

Having that said, deleting everything after ; in the path value will make it impossible to introduce new attributes and will in most cases be backwards compatible with existing code using js-cookie. Maybe that's a pragmatic approach even if the API silently accepts a value that cannot, and will not, be set.

[macintorsten]

@carhartl

I’m wondering how document.cookie = ... treats RFC 6265 non compliant input.

Unrecognized attributes are ignored.

document.cookie = "foo=bar; path=/baz;xyz"

Is treated as:

document.cookie = "foo=bar; path=/baz"

xyz starts a new attribute key-value pair which is unrecognized, hence ignored.

6. Process the attribute-name and attribute-value according to the
   requirements in the following subsections. (Notice that
   attributes with unrecognized attribute-names are ignored.)

https://tools.ietf.org/html/rfc6265

[carhartl]

The value will only be set for the 'path' attribute and not be able to affect any other attribute, regardless of value passed to it. The developer does not necessarily know how the API is implemented.

This is a convincing reason for doing the truncating ourselves. :)

[carhartl]

I’m not against it, but where do we stop sanitizing attributes? Once we go down that route, wouldn’t it be expected that unexpected/invalid values for expires in particular will be rejected, improper domains for domain etc.?

[FagnerMartinsBrack]

@carhartl I agree with truncating the attribute AND adding a bullet point to build a debug tool later if we can.

In regards to other sanitizing, perhaps we can address them on a per request basis? That would be the most pragmatic approach IMHO.

[FagnerMartinsBrack]

I've created a Pull Request on #400.

@carhartl @macintorsten can you guys please review to make sure I'm not missing anything? Thanks.

[FagnerMartinsBrack]

Closing as #400 has landed.