Re: Best Practices comments

Hi Pratik,

Thanks for incorporating my feedback. I have some more comments, and 
they are mostly on specific text or editorial related.

One general comment. I think we should avoid making strict 
recommendations. I would like to use the words such as "Consider 
avoiding XSLT" or "Take care when processing RetrievalMethod". I think 
we should leave the final decision up to the reader, and we should just 
be advisors, and not make strict rules. Just my two cents. I wonder how 
others feel about that.

* 1 Overview

Although much of the best practices is related to mitigating attacks, 
some are not (such as the namespace section). So I would recommend 
rewording this to state that the best practices cover more than just 
attacks, perhaps simply we can change this sentence:

"This flexibility has the downside of increasing the number of possible 
attacks."

to:

"One of the significant downsides of this flexibility is that it can 
increase the number of possible attacks."

* 2.1 For Implementors: Reduce the opportunities for denial of service 
attacks

Step 1 get the verification key

I suggest changing this to "establish trust in the verification key"

* Best Practice 2: Avoid RetrievalMethod.

Very constrained RetrievalMethods are a lot safer. There may be some 
legitimate advantages to RetrievalMethods, perhaps if you have multiple 
signatures in the same document and you want to avoid duplicate KeyInfo 
certificate entries. So maybe we want to change this BP to "Take care 
when processing RetrievalMethod".

* Best Practice #3: "In addition to verifying an XML Signature, also 
check that the key is valid."

The way this is worded it sound likes you could validate the key after 
you validate the signature, so I suggest just changing this to 
"Establish trust in the verification/validation key". (You already 
mentioned the BP about order of steps so I don't think you need to 
repeat that here).

A couple of other comments on the previous paragraph:

"If the KeyInfo is a X509Certificate element, the certificate needs to 
be validated by checking the signature of the certificate's issuer, the 
expiry date, purpose of the certificate, and also make sure that it has 
not been revoked."

These are just some of the checks, not an exhaustive list. I might 
reference RFC 3280/5280 or simply change this to "If the KeyInfo is a 
X509Certificate element, the certificate needs to be validated. This 
involves verifying information in the certificate (for example, the 
expiration date, the purpose of the certificate, checking that it is not 
revoked, etc), and potentially building and validating a chain of 
certificates to a trusted certificate authority. See RFC 3280 for more 
information."

"However if the KeyInfo is a RSA or DSA KeyValue, then there is no way 
to validate the key, so these should not be normally trusted."

That's not totally true, as the keys may have been exchanged and 
verified OOB. I agree on avoiding KeyValues, but unless we elaborate on 
this some more, I would probably omit this sentence.

Also, validation of the key is typically more than an implementation 
issue, and often involves application specific information. I'm not sure 
if we want to mention that though. I think the point is clear enough.

* 2.1.1

s/The following XSLT transform does 4 levels of nested loops/The 
following XSLT transform contains 4 levels of nested loops

s/and in each loop it goes over all the nodes/and for each loop it 
iterates over all the nodes

s/an application server to spend hours processing this it/an application 
server to spend hours processing it

In the sentence:

"To totally eliminate this kind of attack an implementation can choose 
to not support XSLT at all."

I suggest also adding this text:

or provide a mechanism to allow the application to optionally disable 
support for it.

(This puts the burden on the application developer to know what they are 
doing, but making it optional or providing some way to enable/disable it 
is more API/toolkit-friendly). It could also be disabled out of the box, 
but need to be explicitly enabled.

* 2.1.2

s/The following XPath Transforms/The following XPath Transform

I would add the same text above in this section about also providing a 
way to optionally enable/disable the XPath Transform.

This section should also recommend using the XPath Filter Transform 
instead, which I believe avoids the DOS issue in the example. This is 
more of an application issue, but I think it should still mention it, as 
it was designed to address these expensive processing issues with the 
XPath transform.

2.1.3

so it is best to not support RetrievalMethod at all.

As mentioned earlier, I think that is too strong of a recommendation.


more comments later ...


--Sean

Pratik Datta wrote:
> I have updated the best practices document to reflect the discussion 
> that Sean and I were having.
> http://www.w3.org/2007/xmlsec/Drafts/xmldsig-bestpractices/
> 
> The main changes are
> 
>     * reorganization by audience - implementors or app developers.
>     * put in a lot of "why" before each best practice.
> 
> I have not yet changed the timestamp section to incorporate Juan 
> Carlos's comments.
> 
> Pratik
> 
> Sean Mullan wrote:
>>
>> Pratik Datta wrote:
>>>
>>> Sean,
>>>
>>> I am not trying to cover any policy related issues here.
>>
>> But it is kind of a policy issue, right? This is similar to WS 
>> Security Policy where there are constraints that are required to be 
>> satisfied. One of those constraints is to specify which parts of the 
>> document must be signed. So a validating (and signing) implementation 
>> must ensure those requirements are satisfied. Am I off?
>>
>>> The main issue, as you said, is just to see what was signed.
>>
>> I now think they are related but a little different. "See what is 
>> signed" is more applicable to something processing and acting on the 
>> actual data that has been signed (and doesn't necessarily care what 
>> parts of the document were signed), whereas ensuring that certain 
>> elements are signed is more of a lower-level security related issue 
>> where the user isn't really involved (as you say below it is more of a 
>> programmatic issue). Anyway, I think they are both subject to the same 
>> sort of attacks when using arbitrary transforms. So the resulting best 
>> practice should cover both issues :)
>>
>>>
>>> I would like to know what kind of hooks can be put in the 
>>> implementation code to cache and return data.
>>>
>>> One idea is to get the bytes just before digesting and return that as 
>>> an array of bytes.  However this approach is only useful for visual 
>>> comparison, and can't be used for programmatic comparison - how would 
>>> you produce the other array of bytes to compare with?  Also this 
>>> approach is susceptible to wrapping attacks.
>>>
>>> The other approach that I can think of (and this is what the Oracle 
>>> implementation uses) is to run all the transforms except the last 
>>> C14N transform, which may be implicit - the output of this should be 
>>> nodeset. Then one can use the DOM3 method  isSameNode() to compare 
>>> each node of the expected nodeset with the nodeset that is being 
>>> signed. The expected nodeset should be a subset of the nodeset being 
>>> signed. The restriction with approach is that there should not be any 
>>> sequences of  transforms that converts from xml to binary and then 
>>> back to xml, because in that case signed nodeset belongs to a 
>>> different document, and will not match with the expected nodeset.
>>
>> So maybe another best practice would be to advise that implementations 
>> have APIs to return this data (pre-digested and pre-c14n data).
>>
>> --Sean
>>
>

Received on Tuesday, 10 June 2008 17:13:23 UTC