search

simplesamlphp / saml2

SimpleSAMLphp low-level SAML2 PHP library
https://www.simplesamlphp.org
GNU Lesser General Public License v2.1
284 stars 134 forks source link

\r\n in ADFS claims causes message signature to NOT validate correctly. #17

Closed treyhyde closed 9 years ago

treyhyde commented 10 years ago

ADFS appears to apply a message digest that is NOT compliant with the basic XML parsing and C14N standards. \r\n that appears in AttributeStatements (via ADFS outgoing claims) are used to calculate the message digest. For example, attributes that are commonly multi-line like full address fields.

I'll submit some failing unit tests today as well as my VERY awful workaround.

relaxnow commented 10 years ago

Hi, thanks for taking the time to report an issue! Unfortunately as both OpenConext-engine as well as SimpleSAMLphp connect with a lot of different ADFS instances and have never run into this issue I must be quite skeptical. Also I don't see any way that the digest can fail on a multi-line attribute statement. So I'm closing this issue for now until there is a concrete test case. Feel free to reopen or submit a new issue with more info!

treyhyde commented 10 years ago

I sent a pull request with demonstration. Both of these assertions generated with a vanilla testbed ADFS (customers encountered this in the wild with address fields and appropriate claims)

treyhyde commented 10 years ago

@relaxnow Any additional thoughts here?

relaxnow commented 10 years ago

Hi @treyhyde, sorry this took so long (just back from vacation). Thank you for the testcase! I checked this out but this problem seems to be out of scope for the saml2 library. ADFS is generating a digest (hash) over but sending \r\n? Seems odd, maybe in this case simplesamlphp is substituting the entities automagically (http://www.php.net/manual/en/class.domdocument.php#domdocument.props.substituteentities ?)

Could you confirm that what goes over the wire to SSP does indeed contain the entities?

Perhaps @jaimeperez, @olavmrk or even @pmeulen could comment on this? It just seems so odd that with all the ADFS setups we've never come across this issue.

jaimeperez commented 10 years ago

Hi all,

I've never heard of such a problem with ADFS, with one exception. In that case, seems like ADFS didn't like SAML metadata with line feeds or indentations. If their digest mechanism is broken and it doesn't deal correctly with indentation and line feeds, that could explain both issues. Maybe this bug is related only to newer versions of ADFS?

relaxnow commented 10 years ago

Talked this through with @pmeulen , having an AttributeValue with XML encoded values is pretty rare so we may have never seen it.

@treyhyde If you could give us the base64 encoded POST body we could verify that the XML encoded value is in the document as it is posted to SSP (meaning that SSP somewhere somehow filters it out before calculating the digest).

treyhyde commented 10 years ago

@relaxnow Yes, this isn't a case where SSP or even PHP's XML parser are at fault. This is really a compatibility thing. I put some awful string replace hacks in to repair this in our production env.

These aren't XML encoded values... that's just the way to repair it to trick PHP into retaining CRs for in the parsed DOM so you can match ADFS digest algorithm.

I'll see what I can do about pulling the BASE64.

relaxnow commented 10 years ago

Well, as I see it either SSPs use of the XML parser is not conforming to the spec by calculating the digest over the decoded document (which sounds plausible) OR ADFS is not conforming by calculating the digest over the encoded document (sounds less plausible). I don't know the spec well enough to be able to say which should be done.

Only thing I can quickly find is: http://www.w3.org/TR/xmldsig-core/#sec-XML-1 which says that line endings in XML 1.0 should be c14ned into LF characters, which would mean that ADFS is doing it wrong but which wouldn't solve our problems :confounded:

The base64 would at least give us an answer as to whether ADFS is actually even sending us the encoded document (I'm assuming it is, but you know how it is with assumptions).

If it isn't then that's a clear bug in ADFS and should be reported. We would be very interested in which version has that bug so we can warn our respective users. It if is then that would indicate that SSP (and OpenConext-engineblock most likely) are at fault and should likely be fixed. (Though in that case we're assuming that ADFSes behaviour is valid which I can't really say without knowing the spec better or asking around with those that do, @olavmrk ?).

relaxnow commented 10 years ago

Sorry about the extra work by the way and thanks for helping us @treyhyde !

relaxnow commented 9 years ago

Closing this issue until we can confirm with the base64 encoded post values if the encoded document was transmitted.