simplesamlphp / SAML-tracer

Browser extension for examining SAML messages
https://addons.mozilla.org/nl/firefox/addon/saml-tracer/
BSD 2-Clause "Simplified" License
141 stars 39 forks source link

When Using Mask Values option not all Set-Cookie headers are masked #88

Closed LahiruLS closed 10 months ago

LahiruLS commented 10 months ago

There are two issues which I identified.

  1. Due to logic in here[1], only the "Set-Cookie" headers are hashed. However the server could send this in the lower case as "set-cookie" or mixed format too. As the web browser is accepting any kind we needs to improve the logic here to first lowercase the header name and then compare and hash. This would hash any.
  2. Another issue is only the first cookie with "Set-Cookie" header value is hashed. There could be multiple headers with the name "Set-Header" this is allowed with the modern web browsers and this is not handled. in the here[2], we may have to loop the collection and apply the new hashed value as there could be multiple headers with the same name.

[1] - https://github.com/simplesamlphp/SAML-tracer/blob/c5f77e95f2b24edce79bf7b25f1e3af850b01ab9/src/SAMLTraceIO.js#L160 [2] - https://github.com/simplesamlphp/SAML-tracer/blob/c5f77e95f2b24edce79bf7b25f1e3af850b01ab9/src/filters.js#L112

tvdijen commented 10 months ago

Both seem valid issues @LahiruLS ! Thanks for reporting this!

khlr commented 10 months ago

Hmm, I probably broke that functionality back then... https://github.com/simplesamlphp/SAML-tracer/commit/52274e3b53d56abfe57c819daad888302fe5e346#diff-d30457b2e1b5422b9c0169954074c11548c83bcd07f6ca7c9d1f551660a662c1

BTW: I'll have a look at this.

khlr commented 10 months ago

Just a quick update: I haven't changed anything yet, but just did some research. In particular I looked after your second point (multiple headers). And I noticed a very unpleasant peculiarity... There is once again a difference in the implementation of the webRequest API between Firefox and Chromium. While the latter doesn't seem to make any changes to the headers, the implementation in Firefox is different...

So two headers like this yield different output in Firefox and Chromium:

set-cookie: cookie1=a
set-cookie: cookie2=b

Chromium yields this:

key = "set-cookie"
value = "a, b"

Whereas Firefox' webRequest-API generates:

key = "set-cookie"
value = "a\nb"

Firefox treats some headers in a special way. Set-Cookie is one of them. These headers' values get merged using a newline (\n). Why? I have no idea at this moment. But to me it looks like a fundamental change to the "real" payload...

How this header-merging gets done is explained here. And here is the actual source code, that adds the newline.

At this moment I'm not quite sure, what we should do with this information. I'm not a big fan of manually "correcting" the "falsy" webRequest-API's behaviour in our code. But right now this leads to fundamentally divergent output when exporting traces using FF or Chrome, because our hash-functionality doesn't take the newline as a separator into account...

Sample output for a request to https://sustainsys.com/ using Chrome (more precisely Edge) and exporting the hashed trace:

...
{
  "name": "Set-Cookie",
  "value": "ARRAffinity={hash:30faa157a4d8bb3707ee1d79c4dccd0dc1d5bbb16718835ce3a5cfdd49cd1706};Path={hash:8a5edab282632443219e051e4ade2d1d5bbc671c781051bf1437897cbdfea0f1};={hash:a08203914d84eb7ebef95b431c8eca691db25b50ef5ae157182beacba98d2f6a};={hash:1bced1d0ce55892e92065ce28610a61efc9b9a4d7b73d8d2beaf7088ad0d6592};Domain={hash:37495f91e93660c38a51099ac9728d23016c7f040e4d9db9362797b5e63de733}"
},
{
  "name": "Set-Cookie",
  "value": "ARRAffinitySameSite=154131ecdfc01909f829eaee7eb1ac0874419aa8cae06eb9578dddb90f0da2af;Path=/;HttpOnly;SameSite=None;Secure;Domain=sustainsys.com"
},
...

and the same for Firefox:

{
  "name": "Set-Cookie",
  "value": "ARRAffinity={hash:30faa157a4d8bb3707ee1d79c4dccd0dc1d5bbb16718835ce3a5cfdd49cd1706};Path={hash:8a5edab282632443219e051e4ade2d1d5bbc671c781051bf1437897cbdfea0f1};={hash:a08203914d84eb7ebef95b431c8eca691db25b50ef5ae157182beacba98d2f6a};={hash:1bced1d0ce55892e92065ce28610a61efc9b9a4d7b73d8d2beaf7088ad0d6592};Domain={hash:1f87da9477135678446f4b5538c19fd5774a4f5baef9b8fc142fdc5cdd3c8e6e};Path={hash:8a5edab282632443219e051e4ade2d1d5bbc671c781051bf1437897cbdfea0f1};={hash:a08203914d84eb7ebef95b431c8eca691db25b50ef5ae157182beacba98d2f6a};SameSite={hash:dc937b59892604f5a86ac96936cd7ff09e25f18ae6b758e8014a24c7fa039e91};={hash:1bced1d0ce55892e92065ce28610a61efc9b9a4d7b73d8d2beaf7088ad0d6592};Domain={hash:37495f91e93660c38a51099ac9728d23016c7f040e4d9db9362797b5e63de733}"
},

Note that there's only one entry due to the newline not being considered a valid seperator... Part of the second cookie is "merged" into the single entry...

khlr commented 10 months ago

Regarding the \n in Firefox' webRequest-API: There are these two bug reports that exactly describe what we've experienced in this issue. But I'm afraid that there won't be fix anytime quite soon... https://bugzilla.mozilla.org/show_bug.cgi?id=1608979 https://bugzilla.mozilla.org/show_bug.cgi?id=1808069

LahiruLS commented 10 months ago

Awesome. Thanks for the quickest responses. Is this fix released?

khlr commented 10 months ago

No, it's not released yet. But I think I might put together a release in the next few days; maybe during the weekend. :-)

khlr commented 10 months ago

In order to publish a new version in the Chrome Web Store, it is now necessary to provide a privacy policy. No new version can be published before this.

Off the top of my head, I don't think I can just come up with a legally compliant privacy policy just like that. So, unfortunately, it will probably take a while yet.

I'll see if I can talk to a colleague next week who is up to speed on compliance and privacy.