search

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

Mangled data in export (Chrome) #49

Closed tvdijen closed 5 years ago

tvdijen commented 6 years ago

I've received an export made with SAML-tracer on Chrome that has some seriously mangled data in it:

naamloos

I don't have Chrome myself, so it's a bit difficult to test for me.. Can't reproduce it on Firefox, but I'm pretty sure the IdP didn't produce a token like that, even though it's MS ADFS ;)

I'd rather not post the export-file here, but I can email it to someone personally if desired.

khlr commented 6 years ago

This is probably an encoding issue. I think it's related to the manual parsing of the formdata that's required in the chrome version (see here).

tvdijen commented 6 years ago

And that hasn't been released yet, I guess?

khlr commented 6 years ago

The manual parsing is a required part of v1.5, so that's released. But I wasn't aware of this (presumable) encoding issue. Hence there's something to do to avoid the mangled output.

tvdijen commented 6 years ago

But the code you're referring to is an unmerged PR... Looking at it, that's likely going to solve this issue

khlr commented 6 years ago

I had better linked to this commit: https://github.com/UNINETT/SAML-tracer/commit/c5da8965df166e542b6b969838dd450f1f5b79ca

Sorry to disappoint you, but it's already merged 😐 I think this issue requires a bit additional effort. I will have look into it as soon as I find some time for that (may take some time).

khlr commented 6 years ago

@tvdijen, do I still have access to your test-IdP? I'm really embarrassed, but it seems that I've somewhat mislaid the credentials... 🙈 Would you be so kind to supply them again to me?

tvdijen commented 6 years ago

https://github.com/UNINETT/SAML-tracer/issues/26#issuecomment-349116172 These should still work.. I've overwriten the pw again to be sure

khlr commented 6 years ago

Thanks a ton, Tim!

khlr commented 6 years ago

I think, I found the error. It was an incorrect way of decoding application/x-www-form-urlencoded values inside the function postStringToFormDataObject (see the fixing commit https://github.com/UNINETT/SAML-tracer/pull/46/commits/a09c82bbddf7b64a64a39c7657fac07dc5168880).

When I implemented that function, I only tested it against a WS-Federation IDP. The replacement of plusses with whitespaces was fine in this context, since WS-Federation payload (the token; RequestSecurityTokenResponse) is only url-encoded. But SAML protocol has its SAMLResponse POST parameter additionally base64 encoded. Hence the replacement of plusses with whitespaces let the subsequent base64-decoding fail.

I put the fix on top of PR #46 as I think it's really urgent to integrate that fix, too.

Could anyone have yet another look on the now enhanced PR #46, please? (@tvdijen, @thijskh? 🙂) If someone could test this fix using Chrome, that'd be additionally very fine!

@jaimeperez, could then (after a review) release a fixed version of SAML-tracer, please? E.g. version 1.5.1?