metarefresh module does not verify metadata signed with RSA-SHA512 and/or digest SHA512

[restena-sw]

simpleSAMLphp 1.13.2 verifies the exact same metadata just fine if RSA-SHA256 / SHA256 are used, but silently fails (logs signature mismatch) when the metadata is RSA-SHA512 / SHA512.

I would upload a working and non-working sample of metadata XML, but GitHub wouldn't let me. Files are available on request from stefan.winter@restena.lu

[jaimeperez]

Hi Stefan!

Could you please send me both metadata examples to jaime.perez at uninett.no?

Thanks!

[jaimeperez]

Hi again Stefan.

I've been taking a look into this, and I'm inclined to think there's a problem in your metadata. I've made a simple test: create an aggregate with the aggregator2 module using your metadata and resigning. This basically takes your metadata set, removes the certificate and signature information (with signature validation disabled, of course), and signs it again with a keypair of my own. Then I have configured metarefresh to use the URL to that resigned aggregate as the source of metadata, and told it to validate the signature against the certificate I used to sign it in the first place. It works like a charm.

Inspecting the signature, the CanonicalizationMethod, SignatureMethod, Transforms and DigestMethod are exactly the same as in your case. So I think there must be something wrong in your set that's causing the issue.

I'm closing the issue for now. Feel free to reopen it if you have more information at any time.

[restena-sw]

Sorry, I'm not letting you off the hook so easily :-) The same metadata is used in production, and other implementations validate the signature just fine. E.g. the eduGAIN metadata validator (AFAIK Shibboleth in the backend), reports a valid signature (see https://validator.edugain.org/?edugain=LU , tab "Signature"). Also, if I sign the metadata with SHA256, it becomes "correct" in SSP's point of view. That's using the exact same signature application. Your closure comment would indicate that my implementation can correctly sign SHA256, but produces an invalid signature with SHA512, with the invalidity being so subtle that only SSP would pick it up, whereas the signature is valid for other implementations. If you make that claim, I will only accept it if you can pinpoint what exactly the error in the metadata set is. It is also not very surprising that the signature that is created by SSP can subsequently also be validated by SSP. That's all the same implementation, possibly exhibiting the same bug both ways. So, the fact that this works is not a useful counter-argument.

[restena-sw]

I would reopen the bug if I could, but not being a repo collaborator I think I can't.

[jaimeperez]

Sorry Stefan (and Thijs, since you just reopened the issue), but I'm refusing again to bite that hook :wink: I'm closing the issue again, and this time I'm marking it as rejected too.

I wasn't closing the issue based only on the fact that SimpleSAMLphp can validate its own signatures. I'm also taking into account that we've never had before any issues neither creating nor validating SHA512 signatures, and specially the fact that SimpleSAMLphp does not make any differentiation depending on the algorithm used. In fact, SimpleSAMLphp does not care at all about the specifics in the signature. You can check the code that validates the element and verifies the signature and see it for yourself. Just in case: lines 151 - 153 are there to make sure that the right algorithm is used, nothing more.

Besides, if we apply the same reasoning, the fact that other software validates your signature does not mean it's correct. Taking into account we are talking about XML, there's plenty of room for interpretation in the standards to allow room enough for things like this happening. And actually that's what's going on here.

Let's get to the point: the issue here is that the value inside the DigestValue element in your metadata has a line feed in the middle of the digest. The reason why you are observing this exclusively when using SHA512 is because the algorithm produces then a longer output, which the software you are using to create the signature considers convenient to break for some unknown reason.

Line feeds are definitely not part of the valid character set for base64 encoding. You could argue that line feeds are considered white space, but even in that case, white space is not meaningful between brackets, but it is in the content of an element. Normalization would not handle this, since even if we trim all the white space from both the beginning and the end of the value, and collapse white space characters, you would still have a space in between, making the value different than the one you just computed. You could say that a new line here is meaningless since we know this is a base64-encoded value and therefore can be safely removed. And that's true. But, according to the base 64 RFC:

Implementations MUST NOT add line feeds to base-encoded data unless the specification referring to this document explicitly directs base encoders to add line feeds after a specific number of characters.

So what I'm trying to say is: as far as I can see, whether your signature is right or not is subject to interpretation. In any case, SimpleSAMLphp has nothing to do here since we are delegating signature creation and validation to xmlseclibs, and while I see a reason for a more relaxed comparison, I can also see why it is strict in xmlseclibs (*). Not my call anyway.

I can see two options for you here:

• Configure the software you are using to NOT add line feeds to the DigestValue (and SignatureValue, probably), or if that's not possible, change the software you are using.
• Submit an issue to xmlseclibs so that Rob Richards solves your issue by being less strict in the comparison of the digest value.

(*) The XML Signature Syntax and Processing specification says, explicitly:

Comparison of values in reference and signature validation are over the numeric (e.g., integer) or decoded octet sequence of the value. Different implementations may produce different encoded digest and signature values when processing the same resources because of variances in their encoding, such as accidental white space. But if one uses numeric or octet comparison (choose one) on both the stated and computed values these problems are eliminated.

However, bear in mind that even though it might be appealing to let base64_decode() handle the line feeds and compare binary data instead, two different base64-encoded strings can decode into the same binary value (note the end of the strings):

$hash1 = "HEgAwROJOpIcTMqyta7VkKKbt8fqBrZV4RbvnJcCn8NXEiB8ygqX+rfQTqwOOhNH\nJrUFfWoywFIEe+V30zVaug==";
$hash2 = "HEgAwROJOpIcTMqyta7VkKKbt8fqBrZV4RbvnJcCn8NXEiB8ygqX+rfQTqwOOhNHJrUFfWoywFIEe+V30zVauv";
var_dump(base64_decode($hash1) === base64_decode($hash2));
bool(true)

This shouldn't be a problem since this value is part of the SignedInfo element, which is authenticated with public key cryptography. However, it could create room for security issues if an attacker manages to get some document signed that produces an almost identical base64-encoded digest, which depending on the digest is not so unreasonable. A signature from one XML document could be applied to another (different) one and it would still validate, because the binary comparison is positive, even though the base64 strings are not the same.

[restena-sw]

Jaime,

Let's get to the point: the issue here is that the value inside the |DigestValue| element in your metadata has a line feed in the middle of the digest. The reason why you are observing this exclusively when using SHA512 is because the algorithm produces then a longer output, which the software you are using to create the signature considers convenient to break for some unknown reason.

Line feeds are definitely not part of the valid character set for base64 encoding. You could argue that line feeds are considered white space, but even in that case, white space is not meaningful between brackets, but it is in the content of an element. Normalization would not handle this, since even if we trim all the white space from both the beginning and the end of the value, and collapse white space characters, you would still have a space in between, making the value different than the one you just computed. You could say that a new line here is meaningless since we know this is a base64-encoded value and therefore can be safely removed. And that's true. But, according to the base 64 RFC https://tools.ietf.org/html/rfc4648#page-3:

Implementations MUST NOT add line feeds to base-encoded data unless
the specification referring to this document explicitly directs base
encoders to add line feeds after a specific number of characters.

Thanks for this reasoning. It is mostly correct (AFAICT) but doesn't go to the end of the line IMHO. According to XMLDSIG, DigestValue is of type base64Binary (http://www.w3.org/TR/xmldsig-core/#sec-DigestValue). That data type is defined in XML Schema 2, http://www.w3.org/TR/xmlschema-2/#base64Binary which explicitly talks about handling of newlines:

"Note: The above definition of the lexical space is more restrictive than that given in [RFC 2045] http://www.w3.org/TR/xmlschema-2/#RFC2045 as regards whitespace -- this is not an issue in practice. Any string compatible with the RFC can occur in an element or attribute validated by this type, because the ·whiteSpace· http://www.w3.org/TR/xmlschema-2/#dt-whiteSpace facet of this type is fixed to /collapse/, which means that all leading and trailing whitespace will be stripped, and all internal whitespace collapsed to single space characters, /before/ the above grammar is enforced."

So, if there is a newline (no argument that there doesn't /need to/ be any), it is collapsed into a single inline white space /before/ the specific rules of base64String are enforced.

As you correctly state, an inline space is meaningless then, and the base64 value is decoded unchanged.

So, as always, the order of events is important: first the newline becomes a whitespace, and then the string is interpreted as base64, for which the then-whitespace doesn't matter.

So what I'm trying to say is: as far as I can see, whether your signature is right or not is subject to interpretation. In any case, SimpleSAMLphp has nothing to do here since we are delegating signature creation and validation to xmlseclibs https://github.com/robrichards/xmlseclibs, and while I see a reason for a more relaxed comparison, I can also see why it is strict in /xmlseclibs/ (*). Not my call anyway.

That's correct; we're looking at an upstream bug in xmlseclibs. I will open an issue there. If there is a means in GitHub to mark an issue as "on-hold, upstream", could you change the state of this issue accordingly?

I can see two options for you here:

• Configure the software you are using to NOT add line feeds to the |DigestValue| (and |SignatureValue|, probably), or if that's not possible, change the software you are using.
• Submit an issue to xmlseclibs https://github.com/robrichards/xmlseclibs/issues so that Rob Richards solves your issue by being less strict in the comparison of the digest value.

I will go for the latter. Especially since xmlseclibs is mighty inconsistent currently: the other elements of the SignedInfo element are also line-breaked at 76 (SignatureValue and X509Certificate, notably); however the software decodes those correctly (as can be seen when using SHA-256, which still embeds certificate and signing information, but validates nicely) - it only refuses to do it on the DigestValue.

So, either consider line-breaks inside base64Binary data types as invalid -> refuse any document with embedded line breaks; or: consider them as valid, which means a line-breaked DigestValue is just as valid as a line-breaked SignatureValue.

Treating two elements with the same data type differently is internally inconsistent behaviour.

(*) The XML Signature Syntax and Processing specification http://www.w3.org/TR/xmldsig-core/#sec-CoreValidation says, explicitly:

Comparison of values in reference and signature validation are
over the numeric (e.g., integer) or decoded octet sequence of the
value. Different implementations may produce different encoded
digest and signature values when processing the same resources
because of variances in their encoding, such as accidental white
space. But if one uses numeric or octet comparison (choose one) on
both the stated and computed values these problems are eliminated.

However, bear in mind that even though it might be appealing to let |base64_decode()| handle the line feeds and compare binary data instead, two different base64-encoded strings can decode into the same binary value (note the end of the strings):

$hash1 = "HEgAwROJOpIcTMqyta7VkKKbt8fqBrZV4RbvnJcCn8NXEiB8ygqX+rfQTqwOOhNH\nJrUFfWoywFIEe+V30zVaug=="; $hash2 = "HEgAwROJOpIcTMqyta7VkKKbt8fqBrZV4RbvnJcCn8NXEiB8ygqX+rfQTqwOOhNHJrUFfWoywFIEe+V30zVauv"; var_dump(base64_decode($hash1) === base64_decode($hash2)); bool(true)

This shouldn't be a problem since this value is part of the |SignedInfo| element, which is authenticated with public key cryptography. However, it could create room for security issues if an attacker manages to get some document signed that produces an almost identical base64-encoded digest, which depending on the digest is not so unreasonable. A signature from one XML document could be applied to another (different) one and it would still validate, because the binary comparison is positive, even though the base64 strings are not the same.

That part of the reasoning is incorrect IMHO - DigestValue has an exact length in bits after the decoding, and the number of bits to expect is known because the algorithm is spelled out explicitly. Any whitespace changes in the encoding either lead to the correct 512 bits, or the result is plainly visible not a SHA-512 bit value. So there is no security issue. Decoding into binary data makes the expected and present values comparable (they could have any sort of white space); decoding to binary values is the poor man's normalisation.

Also, the same software uses base64_decode for SignatureValue, but not DigestValue; claiming that one is a security issue and the other not does not make much sense. See source code of xmlseclibs at: https://github.com/robrichards/xmlseclibs/blob/master/src/XMLSecurityDSig.php

line 662: return $objKey->verifySignature($this->signedInfo, base64_decode($sigValue));

line 298: return ($digValue == $digestValue);

— Reply to this email directly or view it on GitHub https://github.com/simplesamlphp/simplesamlphp/issues/227#issuecomment-125213707.

[restena-sw]

Hello,

the issue is now open at:

https://github.com/robrichards/xmlseclibs/issues/62

Greetings,

Stefan Winter

On 27.07.15 15:53, Jaime Pérez Crespo wrote:

Sorry Stefan (and Thijs, since you just reopened the issue), but I'm refusing again the bite that hook :wink: I'm closing the issue again, and this time I'm marking it as rejected too.

I wasn't closing the issue based /only/ on the fact that SimpleSAMLphp can validate its own signatures. I'm also taking into account that we've never had before any issues neither creating nor validating SHA512 signatures, and specially the fact that SimpleSAMLphp does not make any differentiation depending on the algorithm used. In fact, SimpleSAMLphp does not care at all about the specifics in the signature. You can check the code that validates the element https://github.com/simplesamlphp/saml2/blob/master/src/SAML2/Utils.php#L29 and verifies the signature https://github.com/simplesamlphp/saml2/blob/master/src/SAML2/Utils.php#L134 and see it for yourself. Just in case: lines 151 - 153 are there to make sure that the right algorithm is used, nothing more.

Besides, if we apply the same reasoning, the fact that other software validates your signature does not mean it's correct. Taking into account we are talking about XML, there's plenty of room for interpretation in the standards to allow room enough for things like this happening. And actually that's what's going on here.

Let's get to the point: the issue here is that the value inside the |DigestValue| element in your metadata has a line feed in the middle of the digest. The reason why you are observing this exclusively when using SHA512 is because the algorithm produces then a longer output, which the software you are using to create the signature considers convenient to break for some unknown reason.

Line feeds are definitely not part of the valid character set for base64 encoding. You could argue that line feeds are considered white space, but even in that case, white space is not meaningful between brackets, but it is in the content of an element. Normalization would not handle this, since even if we trim all the white space from both the beginning and the end of the value, and collapse white space characters, you would still have a space in between, making the value different than the one you just computed. You could say that a new line here is meaningless since we know this is a base64-encoded value and therefore can be safely removed. And that's true. But, according to the base 64 RFC https://tools.ietf.org/html/rfc4648#page-3:

Implementations MUST NOT add line feeds to base-encoded data unless
the specification referring to this document explicitly directs base
encoders to add line feeds after a specific number of characters.

So what I'm trying to say is: as far as I can see, whether your signature is right or not is subject to interpretation. In any case, SimpleSAMLphp has nothing to do here since we are delegating signature creation and validation to xmlseclibs https://github.com/robrichards/xmlseclibs, and while I see a reason for a more relaxed comparison, I can also see why it is strict in /xmlseclibs/ (*). Not my call anyway.

I can see two options for you here:

• Configure the software you are using to NOT add line feeds to the |DigestValue| (and |SignatureValue|, probably), or if that's not possible, change the software you are using.
• Submit an issue to xmlseclibs https://github.com/robrichards/xmlseclibs/issues so that Rob Richards solves your issue by being less strict in the comparison of the digest value.

(*) The XML Signature Syntax and Processing specification http://www.w3.org/TR/xmldsig-core/#sec-CoreValidation says, explicitly:

Comparison of values in reference and signature validation are
over the numeric (e.g., integer) or decoded octet sequence of the
value. Different implementations may produce different encoded
digest and signature values when processing the same resources
because of variances in their encoding, such as accidental white
space. But if one uses numeric or octet comparison (choose one) on
both the stated and computed values these problems are eliminated.

However, bear in mind that even though it might be appealing to let |base64_decode()| handle the line feeds and compare binary data instead, two different base64-encoded strings can decode into the same binary value (note the end of the strings):

$hash1 = "HEgAwROJOpIcTMqyta7VkKKbt8fqBrZV4RbvnJcCn8NXEiB8ygqX+rfQTqwOOhNH\nJrUFfWoywFIEe+V30zVaug=="; $hash2 = "HEgAwROJOpIcTMqyta7VkKKbt8fqBrZV4RbvnJcCn8NXEiB8ygqX+rfQTqwOOhNHJrUFfWoywFIEe+V30zVauv"; var_dump(base64_decode($hash1) === base64_decode($hash2)); bool(true)

This shouldn't be a problem since this value is part of the |SignedInfo| element, which is authenticated with public key cryptography. However, it could create room for security issues if an attacker manages to get some document signed that produces an almost identical base64-encoded digest, which depending on the digest is not so unreasonable. A signature from one XML document could be applied to another (different) one and it would still validate, because the binary comparison is positive, even though the base64 strings are not the same.

— Reply to this email directly or view it on GitHub https://github.com/simplesamlphp/simplesamlphp/issues/227#issuecomment-125213707.

[jaimeperez]

Hi Stefan! Thanks for opening the issue at the xmlseclibs repo :smiley:

Just a few comments:

Thanks for this reasoning. It is mostly correct (AFAICT) but doesn't go to the end of the line IMHO. According to XMLDSIG, DigestValue is of type base64Binary (http://www.w3.org/TR/xmldsig-core/#sec-DigestValue). That data type is defined in XML Schema 2, http://www.w3.org/TR/xmlschema-2/#base64Binary which explicitly talks about handling of newlines:

Note: *The above definition of the lexical space is more restrictive than that given in [RFC 2045] http://www.w3.org/TR/xmlschema-2/#RFC2045 as regards whitespace -- this is not an issue in practice. Any string compatible with the RFC can occur in an element or attribute validated by this type, because the ·whiteSpace· http://www.w3.org/TR/xmlschema-2/#dt-whiteSpace facet of this type is fixed to /collapse/, which means that all leading and trailing whitespace will be stripped, and all internal whitespace collapsed to single space characters, /before/ the above grammar is enforced.

So, if there is a newline (no argument that there doesn't /need to/ be any), it is collapsed into a single inline white space /before/ the specific rules of base64String are enforced.

You are absolutely correct. I was missing the base64Binary definition, my bad. In any case, the key here is not the fact that spaces are collapsed (that's common behaviour for any element's content), but the fact that the white space should be removed. Here's the text:

The length of a base64Binary value is the number of octets it contains. This may be calculated from the lexical form by removing whitespace and padding characters and performing the calculation shown in the pseudo-code below:

lex2 := killwhitespace(lexform) -- remove whitespace characters

The problem here is this text is talking about calculating the length of the value, instead of explicitly calculating the canonical base64 value that should be base for comparisons. I find the wording particularly misleading. And it can get even worse:

Note: For some values the canonical form defined above does not conform to [RFC 2045], which requires breaking with linefeeds at appropriate intervals.

Should we understand from this that values conformant to RFC 2045 are not suitable to be used as base64Binary values?

Anyway. It's pretty evident that space should be ignored, though I still think it is not explicit, opening up for interpretations.

That's correct; we're looking at an upstream bug in xmlseclibs. I will open an issue there. If there is a means in GitHub to mark an issue as "on-hold, upstream", could you change the state of this issue accordingly?

Not that I know of. Unfortunately, github's issue tracker is way less feature-rich than other issue trackers. In any case, this is not an issue in SimpleSAMLphp, so there's not much that we can do, apart from referencing it: robrichards/xmlseclibs#62

I will go for the latter. Especially since xmlseclibs is mighty inconsistent currently: the other elements of the SignedInfo element are also line-breaked at 76 (SignatureValue and X509Certificate, notably); however the software decodes those correctly (as can be seen when using SHA-256, which still embeds certificate and signing information, but validates nicely) - it only refuses to do it on the DigestValue.

So, either consider line-breaks inside base64Binary data types as invalid -> refuse any document with embedded line breaks; or: consider them as valid, which means a line-breaked DigestValue is just as valid as a line-breaked SignatureValue.

Treating two elements with the same data type differently is internally inconsistent behaviour.

No, it is not. Both cases are different and you cannot compare them (no pun intended :smile: )

The X509Certificate has nothing to do here. You could say it is ignored in practice. It is the calling application the one that provides a certificate for xmlseclibs to use in validations. Apart from that, line feeds are just taken care of by canonicalization.

The SignatureValue is also completely different. There's no comparison between values there, because the base64-decoded content is a ciphertext that needs to be decrypted, not compared. So there's no binary comparison here in contrast to the DigestValue. My guess is that xmlseclibs is base64-decoding the value in line 662 just for convenience, because when you call openssl_verify() the parameter there is the signature in binary form.

That part of the reasoning is incorrect IMHO - DigestValue has an exact length in bits after the decoding, and the number of bits to expect is known because the algorithm is spelled out explicitly.

No, the number of bits to expect is known because of the length of the original base64-encoded string. The algorithm used is meaningless here. What you are saying is that comparing two values after applying base64_decode() to them is aware of the algorithm, which is not.

Any whitespace changes in the encoding either lead to the correct 512 bits, or the result is plainly visible not a SHA-512 bit value. So there is no security issue. Decoding into binary data makes the expected and present values comparable (they could have any sort of white space); decoding to binary values is the poor man's normalisation.

I'm not talking about spaces here. I'm talking about the actual base64-encoded value, say the canonical form. Two different canonical base64-encoded strings yield the same decoded value. Taking a closer look at base64, that's normal, as the misalignment between octets and sextets makes you discard the bits remaining after consuming the last bytes. This means the last base64 character can have any value, provided that the most significant bits taken into account for the last decoded byte are kept the same.

So in this case binary comparison should be safe, but I still wonder if there could be a way to exploit this somehow. I'm not claiming there's a security issue, just wondering about the implications of having two different digest strings validating as equal. Hopefully it's not a problem at all.

Also, the same software uses base64_decode for SignatureValue, but not DigestValue; claiming that one is a security issue and the other not does not make much sense. See source code of xmlseclibs at: https://github.com/robrichards/xmlseclibs/blob/master/src/XMLSecurityDSig.php

line 662: return $objKey->verifySignature($this->signedInfo, base64_decode($sigValue));

As I said before, both cases have nothing to do with each other. What I was wondering about is binary comparisons of base64-decoded strings, not about deciphering a base64-encoded ciphertext.

Anyway, let's see if we can get this solved in xmlseclibs. Meanwhile, I'd recommend you to change the software you use to sign, since this will take some time to be resolved in a stable release.

[restena-sw]

I have meanwhile checked the issue with the author of xmlseclibs. There is now a fix in the 1.4 branch and master of xmlseclibs. The next release version with the fix in it will be 1.4.1.

You ship an embedded version of xmlseclibs in simpleSAMLphp. How about if we now re-open the bug, this time with a description of

"The version of xmlseclibs shipped with simpleSAMLphp is outdated, and effectively prevents the use of SHA-512 digests when using metarefresh. Please ship the next version of simpleSAMLphp with xmlseclibs 1.4.1 (if it is released by the time) or the current tip of branch 1.4 of that product."

:-)

Since the author wrote that 1.4 is a drop-in replacement for 1.3.x, I will test this with simpleSAMLphp 1.13.2 and will let you know if its current 1.4 branch version works for me.

Greetings,

Stefan Winter

On 28.07.2015 11:32, Jaime Pérez Crespo wrote:

Hi Stefan! Thanks for opening the issue at the xmlseclibs repo :smiley:

Just a few comments:

Thanks for this reasoning. It is mostly correct (AFAICT) but
doesn't go
to the end of the line IMHO. According to XMLDSIG, DigestValue is of
type base64Binary
(http://www.w3.org/TR/xmldsig-core/#sec-DigestValue).
That data type is defined in XML Schema 2,
http://www.w3.org/TR/xmlschema-2/#base64Binary which explicitly talks
about handling of newlines:

    Note: *The above definition of the lexical space is more
    restrictive
    than that given in [RFC 2045]
    http://www.w3.org/TR/xmlschema-2/#RFC2045 as regards whitespace --
    this is not an issue in practice. Any string compatible with
    the RFC can
    occur in an element or attribute validated by this type,
    because the
    ·whiteSpace· http://www.w3.org/TR/xmlschema-2/#dt-whiteSpace
    facet of
    this type is fixed to /collapse/, which means that all leading and
    trailing whitespace will be stripped, and all internal whitespace
    collapsed to single space characters, /before/ the above
    grammar is
    enforced.

So, /if/ there is a newline (no argument that there doesn't /need
to/ be
any), it is collapsed into a single inline white space /before/ the
specific rules of base64String are enforced.

You are absolutely correct. I was missing the base64Binary definition, my bad. In any case, the key here is not the fact that spaces are collapsed (that's common behaviour for any element's content), but the fact that the white space should be removed. Here's the text:

The length of a base64Binary value is the number of octets it
contains. This may be calculated from the lexical form by removing
whitespace and padding characters and performing the calculation
shown in the pseudo-code below:

lex2 := killwhitespace(lexform) -- remove whitespace characters

The problem here is this text is talking about calculating the length of the value, instead of explicitly calculating the canonical base64 value that should be base for comparisons. I find the wording particularly misleading. And it can get even worse:

Note: For some values the canonical form defined above does not
conform to [RFC 2045], which requires breaking with linefeeds at
appropriate intervals.

Should we understand from this that values conformant to RFC 2045 are not suitable to be used as /base64Binary/ values?

Anyway. It's pretty evident that space should be ignored, though I still think it is not explicit, opening up for interpretations.

That's correct; we're looking at an upstream bug in xmlseclibs. I will
open an issue there. If there is a means in GitHub to mark an issue as
"on-hold, upstream", could you change the state of this issue
accordingly?

Not that I know of. Unfortunately, github's issue tracker is way less feature-rich than other issue trackers. In any case, this is not an issue in SimpleSAMLphp, so there's not much that we can do, apart from referencing it: robrichards/xmlseclibs#62 https://github.com/robrichards/xmlseclibs/issues/62

I will go for the latter. Especially since xmlseclibs is mighty
inconsistent currently: the other elements of the SignedInfo
element are
also line-breaked at 76 (SignatureValue and X509Certificate, notably);
however the software decodes those correctly (as can be seen when
using
SHA-256, which still embeds certificate and signing information, but
validates nicely) - it only refuses to do it on the DigestValue.

So, either consider line-breaks inside base64Binary data types as
invalid -> refuse any document with embedded line breaks;
or: consider them as valid, which means a line-breaked DigestValue is
just as valid as a line-breaked SignatureValue.

Treating two elements with the same data type differently is
internally
inconsistent behaviour.

No, it is not. Both cases are different and you cannot compare them (no pun intended :smile: )

The X509Certificate has nothing to do here. You could say it is ignored in practice. It is the calling application the one that provides a certificate for /xmlseclibs/ to use in validations. Apart from that, line feeds are just taken care of by canonicalization.

The |SignatureValue| is also completely different. There's no comparison between values there, because the base64-decoded content is a ciphertext that needs to be decrypted, not compared. So there's no binary comparison here in contrast to the |DigestValue|. My guess is that /xmlseclibs/ is base64-decoding the value in line 662 just for convenience, because when you call |openssl_verify()| the parameter there is the signature in binary form.

That part of the reasoning is incorrect IMHO - DigestValue has an
exact
length in bits after the decoding, and the number of bits to expect is
known because the algorithm is spelled out explicitly.

No, the number of bits to expect is known because of the length of the original base64-encoded string. The algorithm used is meaningless here. What you are saying is that comparing two values after applying |base64_decode()| to them is aware of the algorithm, which is not.

Any whitespace changes in the encoding either lead to the correct
512 bits, or the
result is plainly visible not a SHA-512 bit value. So there is no
security issue. Decoding into binary data makes the expected and
present
values comparable (they could have any sort of white space);
decoding to
binary values is the poor man's normalisation.

I'm not talking about spaces here. I'm talking about the actual base64-encoded value, say the canonical form. Two different canonical base64-encoded strings yield the same decoded value. Taking a closer look at base64, that's normal, as the misalignment between octets and sextets makes you discard the bits remaining after consuming the last bytes. This means the last base64 character can have any value, provided that the most significant bits taken into account for the last decoded byte are kept the same.

So in this case binary comparison should be safe, but I still wonder if there could be a way to exploit this somehow. I'm not claiming there's a security issue, just wondering about the implications of having two different digest strings validating as equal. Hopefully it's not a problem at all.

Also, the same software uses base64_decode for SignatureValue, but not
DigestValue; claiming that one is a security issue and the other not
does not make much sense. See source code of xmlseclibs at:
https://github.com/robrichards/xmlseclibs/blob/master/src/XMLSecurityDSig.php

line 662:
return $objKey->verifySignature($this->signedInfo,
base64_decode($sigValue));

As I said before, both cases have nothing to do with each other. What I was wondering about is binary comparisons of base64-decoded strings, not about deciphering a base64-encoded ciphertext.

Anyway, let's see if we can get this solved in /xmlseclibs/. Meanwhile, I'd recommend you to change the software you use to sign, since this will take some time to be resolved in a stable release.

— Reply to this email directly or view it on GitHub https://github.com/simplesamlphp/simplesamlphp/issues/227#issuecomment-125520915.

[thijskh]

There's definitely the desire to upgrade xmlseclibs to at least 1.4 for SSP 2.0. So your tests would be very welcome.

[restena-sw]

Hi,

There's definitely the desire to upgrade xmlseclibs to at least 1.4 for SSP 2.0. So your tests would be very welcome.

okay, I'm using a local copy of SSP 1.13.2 which did not validate my SHA512 digest before (but did validate my SHA256 one). So this is a good test object.

replacing vendor/simplesamlphp/xmlseclibs/xmlseclibs.php with the 1.4.1 version did NOT work just like that; 1.4.1 complained about a missing include:

require(bla/simplesamlphp-1.13.2/vendor/simplesamlphp/xmlseclibs/src//XMLSecurityKey.php)

require(bla/simplesamlphp-1.13.2/vendor/simplesamlphp/xmlseclibs/src//XMLSecurityDSig.php)

require(bla/simplesamlphp-1.13.2/vendor/simplesamlphp/xmlseclibs/src//XMLSecEnc.php)

Copying those three into the expected place makes both SHA256 and SHA512 work with metarefresh.

This is an acceptable workaround for my installations; but I'd suggest to make a 1.13.3 release with this new package rather than wait for inclusion into a SSP 2.0 release.

Greetings,

Stefan Winter

Stefan WINTER Ingenieur de Recherche Fondation RESTENA - Réseau Téléinformatique de l'Education Nationale et de la Recherche 6, rue Richard Coudenhove-Kalergi L-1359 Luxembourg

Tel: +352 424409 1 Fax: +352 422473

PGP key updated to 4096 Bit RSA - I will encrypt all mails if the recipient's key is known to me

http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xC0DE6A358A39DC66

[jaimeperez]

Thanks for the feedback Stefan!

We will be releasing a 1.14.0 soon, but that implies much more than this, of course. I'll think about having a 1.13.3 release with a bump in the xmlseclibs version as well as other bug fixes.

[restena-sw]

Oh, 1.14 would also do for me :-)

On 05.08.2015 11:16, Jaime Pérez Crespo wrote:

Thanks for the feedback Stefan!

We will be releasing a 1.14.0 soon, but that implies much more than this, of course. I'll think about having a 1.13.3 release with a bump in the /xmlseclibs/ version as well as other bug fixes.

— Reply to this email directly or view it on GitHub https://github.com/simplesamlphp/simplesamlphp/issues/227#issuecomment-127927248.

Stefan WINTER Ingenieur de Recherche Fondation RESTENA - Réseau Téléinformatique de l'Education Nationale et de la Recherche 6, rue Richard Coudenhove-Kalergi L-1359 Luxembourg

Tel: +352 424409 1 Fax: +352 422473

PGP key updated to 4096 Bit RSA - I will encrypt all mails if the recipient's key is known to me

http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xC0DE6A358A39DC66

[jaimeperez]

The only downside in that case is that it will take a bit more time.

How urgent it is for you to have a fix for this in a stable release?

[restena-sw]

Hi,

The only downside in that case is that it will take a bit more time.

How urgent it is for you to have a fix for this in a stable release?

We are just now starting our federation, and I could mandate SHA-512 support (and thus sign our metadata with it) for our customers-to-be if there were commodity software which handles this correctly.

As of right now, the best candidate for "commodity software" is SSP. If it takes too long for a good version of SSP to manifest, I would probably have to go down to signing with SHA-256.

How long is too long? I don't know - depends when we get our first non-internal customer on board. I'm working on it :-)

Stefan

Stefan WINTER Ingenieur de Recherche Fondation RESTENA - Réseau Téléinformatique de l'Education Nationale et de la Recherche 6, rue Richard Coudenhove-Kalergi L-1359 Luxembourg

Tel: +352 424409 1 Fax: +352 422473

PGP key updated to 4096 Bit RSA - I will encrypt all mails if the recipient's key is known to me

http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xC0DE6A358A39DC66

[jaimeperez]

Ok, then I understand there's no big urge, specially since SHA-512 does not provide a significant difference in terms of security compared to SHA-256 (as of today, of course).

Let's do this. I'm going to update right now the dependency on xmlseclibs, and I will hold the update to be released with 1.14.0. But if you have any issue with a possible customer using SSP, let me know here and I'll release 1.13.3 right away.

Does it sound reasonable?

[restena-sw]

Perfect! Thanks!

On 05.08.2015 11:48, Jaime Pérez Crespo wrote:

Ok, then I understand there's no big urge, specially since SHA-512 does not provide a significant difference in terms of security compared to SHA-256 (as of today, of course).

Let's do this. I'm going to update right now the dependency on /xmlseclibs/, and I will hold the update to be released with 1.14.0. But if you have any issue with a possible customer using SSP, let me know here and I'll release 1.13.3 right away.

Does it sound reasonable?

— Reply to this email directly or view it on GitHub https://github.com/simplesamlphp/simplesamlphp/issues/227#issuecomment-127939197.

Stefan WINTER Ingenieur de Recherche Fondation RESTENA - Réseau Téléinformatique de l'Education Nationale et de la Recherche 6, rue Richard Coudenhove-Kalergi L-1359 Luxembourg

Tel: +352 424409 1 Fax: +352 422473

PGP key updated to 4096 Bit RSA - I will encrypt all mails if the recipient's key is known to me

http://pgp.mit.edu:11371/pks/lookup?op=get&search=0xC0DE6A358A39DC66