[Mono-dev] Bug in SignedXml.GetIdElement
Atsushi Eno
atsushieno at veritas-vos-liberabit.com
Tue Jul 23 09:15:54 UTC 2013
Jonathan Gagnon wrote:
>
> On Fri, Jul 19, 2013 at 1:05 PM, Atsushi Eno
> <atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>> wrote:
>
> (2013年07月17日 21:25), Jonathan Gagnon wrote:
>
>
>
> On Tue, Jul 16, 2013 at 12:16 PM, Atsushi Eno
> <atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>
> <mailto:atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>>> wrote:
>
> Jonathan Gagnon wrote:
>
> It does not work when the SAML document is not
> referring to
> any DTD. In my case, I receive the following
> exception when I
> call the CheckSignature method :
>
> System.Security.Cryptography.CryptographicException:
> Malformed
> reference object: [referenceId]
> at
>
> System.Security.Cryptography.Xml.SignedXml.GetReferenceHash
> (System.Security.Cryptography.Xml.Reference r, Boolean
> check_hmac) [0x00000] in <filename unknown>:0
> at
>
> System.Security.Cryptography.Xml.SignedXml.CheckReferenceIntegrity
> (System.Collections.ArrayList referenceList) [0x00000] in
> <filename unknown>:0
> at
>
> System.Security.Cryptography.Xml.SignedXml.CheckSignatureInternal
> (System.Security.Cryptography.AsymmetricAlgorithm key)
> [0x00000] in <filename unknown>:0
> at
> System.Security.Cryptography.Xml.SignedXml.CheckSignature
> (System.Security.Cryptography.AsymmetricAlgorithm key)
> [0x00000] in <filename unknown>:0
> at TestSAML.Program.Main (System.String[] args)
> [0x00000] in
> <filename unknown>:0
>
>
> Of course it happens because you should be processing
> corresponding DTD or XML Schema.
>
>
>
> The same code works in .NET and it does work if I
> modify the
> GetIdElement method to check for "ID".
>
> So in your opinion, I should create a class that
> derives from
> SignedXml and override GetIdElement?
>
>
> I'm not sure I would like to answer yes (if you want to
> have ID
> being processed) or no (you should actually process DTD or
> XSD).
>
>
> I added references to the corresponding XSDs but it doesn't
> seem to help. I'm still getting the same exception.
>
>
> Because you didn't set up XmlDocument properly to process XSDs.
> (You're discussing you're doing right without showing code.)
>
>
>
> You're probably right that I didn't set it up properly. It seems to
> be a poorly documented part of .NET. Do you have a link to a good
> example?
>
> Basically, I tried adding a reference to the xsd inside the SAML
> document but it didn't help. Then I tried the following example
> without success : http://msdn.microsoft.com/en-us/library/ms162371.aspx
>
> I also noticed that calling the Schemas.Add method is very slow
> (several seconds each time), and didn't want that overhead in our
> application.
>
I have no idea why your code doesn't work as you don't show us the exact
code, but for the slowness issue I guess you are loading the relevant
resources (xml schemas and DTDs) every time. Some caching XmlResolver
would save that (use XmlReaderSettings.XmlResolver). XmlSchema
processing itself should not take several seconds.
You can partially verify your code only with XmlDocument by calling
GetElementById() and see if your SAML ID is found. If it's not found,
then SignedXml won't work for you either.
>
>
> It does fix the problem for me. But wouldn't it be
> better to
> modify SignedXml.GetIdElement() to behave more like
> .NET so
> that other users don't encounter the same problem?
>
>
> I don't support any use of API that violates W3C
> specification.
>
>
> >From what I understand, the W3C specification is only about
> the signature part of a signed xml. There is nothing
> regarding other parts of the signed XML, and the SAML standard
> defines the id differently. So I'm not sure that supporting
> SAML ids would violate the W3C specification.
>
>
> I don't understand your discussion. Any additional local
> attributes that do not conform to the XML Schema defined in
> xmldsig specification violates XML schema validation.
>
>
> What I'm saying is that the XML Schema defined in xmldsig
> specification is often applied to a subpart of an XML document. Here
> is an example :
>
> <samlp:Response>
> <saml:Assertion ID="abc">
> <ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
> </ds:Signature>
> ...
> </saml:Assertion>
> </samlp:Response>
>
> So the XML Schema defined by the W3C specification only applies to
> what is inside the <ds:Signature> tags. Thus, it doesn't violate the
> XML Schema to have an "ID" attribute for the <saml:Assertion>.
>
> Also, if you look at some examples on the W3C sites, you will find
> that some of them use "ID" instead of "Id" (that could be a mistake
> though). Like this one :
>
> http://www.w3.org/TR/xmldsig-core/#sec-NamespaceContext
>
This sample elements there are NOT in xmldsig namespace. They are
totally defined by user (and their schema definitions are unknown). Thus
that does not work as an objection to my comment.
Though that rang a bell. I had to rethink about the existing
implementation. I figured that it is buggy in my view either way, by
that it allowed "Id" attribute with no reason.
Hence I tried to delete that XPath search for "Id". It resulted in 3
NUnit test "regressions":
1)
MonoTests.System.Security.Cryptography.Xml.SignedXmlTest.DataReferenceToNonDataObject
: System.Security.Cryptography.CryptographicException : Malformed
reference object: id:1
2) MonoTests.System.Security.Cryptography.Xml.SignedXmlTest.GetIdElement
: System.NullReferenceException : Object reference not set to an
instance of an object
From the failure 2) I figured that we still have to look for
xmldsig:Id, thus merely removing XPath search is wrong; adding namespace
condition in XPath fixed this.
Still, 1) exposed the difference from .NET. I need to find out how
xmldsig:Reference processes "ID" by definition. Long story begins from
here...
It is described at xmldsig section 3.2 Core Validation
http://www.w3.org/TR/xmldsig-core/#sec-CoreValidation
The REQUIRED steps of core validation
<http://www.w3.org/TR/xmldsig-core/#def-ValidationCore> include (1)
reference validation
<http://www.w3.org/TR/xmldsig-core/#def-ValidationReference>,
the verification of the digest contained in each |Reference| in
|SignedInfo|,
and (2) the cryptographic signature validation
<http://www.w3.org/TR/xmldsig-core/#def-ValidationSignature> of the
signature calculated
over |SignedInfo|.
There is very interesting notes immediately below the sentence:
Note, there may be valid signatures that some signature
applications are
unable to validate. Reasons for this include failure to implement
optional parts
of this specification, inability or unwillingness to execute
specified algorithms,
or inability or unwillingness to dereference specified URIs (some
URI schemes
may cause undesirable side effects), etc.
...!? That does not sound very solid. An implementation can
reject/ignore some signatures.
Anyhow, the key specification seems to be (1) reference validation.
Section 3.2.1 says:
Obtain the data object to be digested. (For example, the signature
application
may dereference the |URI| and execute |Transforms| provided by the
signer in the
R|eference| element, or it may obtain the content through other
means such as
a local cache.)
That does not tell anything precise. Let's have a look at the definition
of URI attribute semantics in Reference element, which is in section
4.3.3.1:
The |URI| attribute identifies a data object using a URI-Reference
[URI <http://www.w3.org/TR/xmldsig-core/#ref-URI>].
The mapping from this attribute's value to a URI reference MUST be
performed
as specified in section 3.2.17 of [XMLSCHEMA Datatypes, 2nd Edition
<http://www.w3.org/TR/xmldsig-core/#ref-XML-schema>].
Additionally: Some existing implementations are known to verify the
value of
the URI attribute against the grammar in [URI
<http://www.w3.org/TR/xmldsig-core/#ref-URI>]. It is therefore safest to
perform
any necessary escaping while generating the URI attribute.
We RECOMMEND XML signature applications be able to dereference URIs in
the HTTP scheme. Dereferencing a URI in the HTTP scheme MUST comply
with the Status Code Definitions
<http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3.4> of
[HTTP <http://www.w3.org/TR/xmldsig-core/#ref-HTTP>] (e.g., 302, 305 and
307 redirects
are followed to obtain the entity-body of a 200 status code response).
Applications should also be cognizant of the fact that protocol
parameter and
state information, (such as HTTP cookies, HTML device profiles or
content
negotiation), may affect the content yielded by dereferencing a URI.
So, the specification says that URI reference MUST be performed as in
XML Schema Datatypes Pt.2 (which defines ID type).
There's also some mention on "some existing implementations" and at
first I looked at this, I thought like "oh we can bring some
implementation dependent additional ID semantics". But it only describes
how URI attribute value should be parsed and not to mention any
arbitrary ID attributes can be added.
But is there really no possibility to allow ID? 4.3.3.2 describes how
"#chapter1" is interpreted, as an example:
Identifies a node-set containing the element with ID attribute
value 'chapter1'
of the XML resource containing the signature. XML Signature (and its
applications) modify this node-set to include the element plus all
descendants
including namespaces and attributes -- but not comments.
Here I can claim that the "XML resource" CAN be determined by URI (which
in general I doubt and I could also say "it must be text/xml as it
explicitly says XML" though).
Based on that claim, let's read on - in that context, let's follow the
"recommendation", which says we should follow HTTP scheme. URI
fragmentation, as specified in RFC 3986, says that fragment is MIME type
dependent. Its section 3.5 says:
The semantics of a fragment identifier are defined by the set of
representations
that might result from a retrieval action on the primary resource.
The fragment's
format and resolution is therefore dependent on the media type
[RFC2046] of a
potentially retrieved representation, even though such a retrieval
is only
performed if the URI is dereferenced.
RFC2046 (2045, or 822) doesn't say anything about HTTP specifically.
Instead HTTP RFC claims something in 7.2.1:
Any HTTP/1.1 message containing an entity-body SHOULD include a
Content-Type header field defining the media type of that body. If
and only if
the media type is not given by a Content-Type field, the recipient
MAY attempt
to guess the media type via inspection of its content and/or the
name extension(s)
of the URI used to identify the resource. If the media type remains
unknown,
the recipient SHOULD treat it as type "application/octet-stream".
application/octet-stream means nothing for URI fragment... but it also
says we "may" attempt to "guess" the media type. Here again I believe we
should in general say "it must be text/xml (or application/xml+*)". Yet
I could thinkg of one rational opportunity for "ID" attribute: it is
HTTP so we may assume text/html(!)
What happens if we can say it is text/html? It is *case insensitive* !
In that case, all of "Id", "ID", "id" or "iD" can be (and must be) valid.
This is actually NOT what .NET does; .NET rejects "iD" for no reason (or
not according to this "theory"). I believe that .NET is rather nothing
consistent and it is either buggy or took absurd request to process "ID"
and "id" (but not "iD") from their users.
Anyhow this is the only possible logical way that I could find to
include "ID" as SignedXml.GetIdElement()'s processing target.
(Does this logic brings too much ambiguity on existing "look for Id"
implementation by bringing "valid assumption on ID case insensitivity" ?
I hope not - this logic can be applied to only when URI fragment is
involved and the relevant specification mentions text/html or HTTP as
applicable content type either directly or indirectly. But if it turned
out to be too ambiguous, I'll quickly recall this permissive idea. It is
not my taste anyways.)
Back to the NUnit test failure 1), I added that test while I was
implementing WS-Security stack, so it is actually something that we
should have covered in the WSS implementation by overriding
GetIdElement(), and since we indeed do that, we can safely mark this as
[Ignore] or simply remove that "invalid" test. So we have no trouble on
changing the semantics of GetIdElement() in *more strict* way too.
Atsushi Eno
> Basically, my point is that it seems like there are more that one
> standards of XML signature. SAML is one of them and it defines an ID
> as "ID" instead of "Id". Microsoft seems to have decided to support
> it directly without the need to process the XSDs. I thought it would
> be a good idea to have mono do the same and that is what my patch does.
>
> Jonathan
>
>
> Atsushi Eno
>
>
>
> Though I'm just pointing out the facts. There may be
> people who
> want to take responsibility on the entire XML Signature
> stuff and
> go ahead to apply the changes.
>
> Atsushi Eno
>
> Thanks,
>
> Jonathan
>
>
> On Tue, Jul 16, 2013 at 10:24 AM, Atsushi Eno
> <atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>
> <mailto:atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>>
> <mailto:atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>
> <mailto:atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>>>> wrote:
>
> Whenever SAML document instance refers to its
> schema or
> DTD that
> will validate "ID" attribute as expected, since
> SignedXml
> internally uses XmlDocument.GetElementById () which is
> expected to
> collect "IDs" where "IDs" means a validated ID by
> XmlValidatingReader or any XmlReader that has
> XmlReaderSettings to
> consider XmlSchema or DTD. Hence that does not
> cause any
> problem
> for SAML.
>
> (Also note that SignedXml implementation could
> override
> SignedXml.GetIdElement(). Mono's WCF
> implementation makes
> use of
> it to support WS-Security ID attribute.)
>
> Atsushi Eno
>
> Jonathan Gagnon wrote:
>
> This is true for the signature, but not true
> for SAML
> assertions, where ids are defined as "ID" :
>
> http://schemas.stylusstudio.com/saml/nea261b70/complexType_AssertionType.html
>
> I don't know in which case we would need "id" in
> lowercase,
> but since .NET supports it, there is probably
> a valid
> reason
> for it too.
>
> *Jonathan Gagnon*
> Responsable des architectures systèmes
> 600, boulevard Armand-Frappier, bureau 200
> Laval (Québec) H7V 4B4
> Canada
> T : 450-662-6101 <tel:450-662-6101>
> <tel:450-662-6101 <tel:450-662-6101>> <tel:450-662-6101
> <tel:450-662-6101>
> <tel:450-662-6101 <tel:450-662-6101>>> poste 234
> <http://www.croesus.com>
>
> <http://www.facebook.com/pages/Croesus-Finansoft/345020305606240><http://www.linkedin.com/company/croesus-finansoft?trk=hb_tab_compy_id_26141><https://twitter.com/CroesusFin>
>
>
>
> On Tue, Jul 16, 2013 at 2:30 AM, Atsushi Eno
> <atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>
> <mailto:atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>>
> <mailto:atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>
> <mailto:atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>>>
> <mailto:atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>
> <mailto:atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>>
>
> <mailto:atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>
> <mailto:atsushieno at veritas-vos-liberabit.com
> <mailto:atsushieno at veritas-vos-liberabit.com>>>>> wrote:
>
> W3C XML Signature specification explicitly
> "Id" as
> the valid
> attribute name for referencing an element,
> by its XML
> Schema and DTD:
> http://www.w3.org/TR/xmldsig-core/#sec-Signature
> http://www.w3.org/TR/xmldsig-core/#sec-SignatureValue
> http://www.w3.org/TR/xmldsig-core/#sec-SignedInfo
> http://www.w3.org/TR/xmldsig-core/#sec-Reference
> http://www.w3.org/TR/xmldsig-core/#sec-KeyInfo
> http://www.w3.org/TR/xmldsig-core/#sec-Object
> http://www.w3.org/TR/xmldsig-core/#sec-Manifest
> http://www.w3.org/TR/xmldsig-core/#sec-SignatureProperties
>
> If Microsoft treats "id" or "ID"
> attributes as if
> they were ID
> (and not "iD" ?), they will have to fix
> their bug.
>
> Atsushi Eno
>
> (2013年07月12日 23:58), Jonathan Gagnon wrote:
>
> I have encountered a bug similar to 4938
>
> <https://bugzilla.xamarin.com/show_bug.cgi?id=4938>.
>
>
> My problem is that mono does not find the
> reference id
> because
> the id is in uppercase ('ID' instead
> of 'Id').
> This works
> correctly on .NET.
>
> As stated in the bug description, the
> problem
> is in the
> SignedXml class, GetIdElement method.
>
> I wrote a very simple patch that fixes the
> problem by
> looking
> for "id" and "ID". Should I do a pull
> request with
> that fix?
>
> *Jonathan Gagnon*
>
> Responsable des architectures systèmes
> 600, boulevard Armand-Frappier, bureau 200
> Laval (Québec) H7V 4B4
> Canada
> T : 450-662-6101 <tel:450-662-6101>
> <tel:450-662-6101 <tel:450-662-6101>>
> <tel:450-662-6101 <tel:450-662-6101> <tel:450-662-6101
> <tel:450-662-6101>>> <tel:450-662-6101 <tel:450-662-6101>
> <tel:450-662-6101 <tel:450-662-6101>>
>
> <tel:450-662-6101 <tel:450-662-6101>
> <tel:450-662-6101 <tel:450-662-6101>>>> poste 234
>
>
> <http://www.croesus.com>
>
> <http://www.facebook.com/pages/Croesus-Finansoft/345020305606240><http://www.linkedin.com/company/croesus-finansoft?trk=hb_tab_compy_id_26141><https://twitter.com/CroesusFin>
>
>
>
>
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> <mailto:Mono-devel-list at lists.ximian.com>
> <mailto:Mono-devel-list at lists.ximian.com
> <mailto:Mono-devel-list at lists.ximian.com>>
> <mailto:Mono-devel-list at lists.ximian.com
> <mailto:Mono-devel-list at lists.ximian.com>
> <mailto:Mono-devel-list at lists.ximian.com
> <mailto:Mono-devel-list at lists.ximian.com>>>
>
> <mailto:Mono-devel-list at lists.ximian.com
> <mailto:Mono-devel-list at lists.ximian.com>
> <mailto:Mono-devel-list at lists.ximian.com
> <mailto:Mono-devel-list at lists.ximian.com>>
> <mailto:Mono-devel-list at lists.ximian.com
> <mailto:Mono-devel-list at lists.ximian.com>
> <mailto:Mono-devel-list at lists.ximian.com
> <mailto:Mono-devel-list at lists.ximian.com>>>>
> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>
>
>
>
>
>
>
>
>
>
More information about the Mono-devel-list
mailing list