Re: Best Practices comments

I have included this in the draft.

In some cases instead of using your exact edits, I have made slightly 
different changes to convey your intent better, see below.

Sean Mullan wrote:
>
> 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."
>
Changing this sentence doesn't imply that the best practices document 
covers more than attacks. So I have added a sentence "While most of 
these best practices are related to mitigating attacks, some are for 
other issues - e.g. signing xml that doesn't use namespaces."

> * 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"
I changed it to "fetch the verification key and establish trust in it". 
so that it includes my original intent of fetching the key from the 
KeyInfo, as well as validating the certificate.
>
> * 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.
>
In general I think it is better to clarify our advice rather than omit 
it. So I have changed this sentence to
"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; unless the 
keys has already been exchanged out of band, and the implementation only 
uses the KeyInfo to compare against the OOB exchanged key"
> 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.
This is an important point, I have mentioned it - in fact we should make 
it more explict in the future  e.g. saying that the implementation 
should provide callbacks for the application.
> * 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 Monday, 23 June 2008 08:41:37 UTC