ietf-wg-httpapi / mediatypes

Other
5 stars 4 forks source link

Secdir review #81

Closed ioggstream closed 1 year ago

ioggstream commented 1 year ago

Secdir review

The security considerations section refers to section 4.6 of RFC 6838 and possible exploits regarding arbitrary code execution from YAML tags, DoS through infinite or high recursion, and DoS through the partial processing of YAML streams.
I do agree with each of the mitigations prescribed for the aforementioned exploits, but it does seem counterintuitive to me to validate all the documents in the stream before processing. Does this defeat the purpose of streaming?

The term "YAML stream" designates one or more serialized YAML documents, independently on how they are processed (e.g. a stored file is a YAML stream). While you are correct that the suggestions are counter-intuitive when processing YAML streams on the fly, we are not implying that "YAML streams" should be streamed. Instead, we are just using the YAML terminology.

CC: @eemeli could you please check if this reply is correct?

Editorial Comments:

s/Security considerations: See Section 2.1/Security considerations: See Section 4/

This should be: "Same as application/yaml".

s/impact on the/impact the/ s/serialize it JSON/serialize it in JSON/

This should be: "Same as application/yaml".

s/details: this/details, which/

eemeli commented 1 year ago

I do agree with each of the mitigations prescribed for the aforementioned exploits, but it does seem counterintuitive to me to validate all the documents in the stream before processing. Does this defeat the purpose of streaming?

The term "YAML stream" designates one or more serialized YAML documents, independently on how they are processed (e.g. a stored file is a YAML stream). While you are correct that the suggestions are counter-intuitive when processing YAML streams on the fly, we are not implying that "YAML streams" should be streamed. Instead, we are just using the YAML terminology.

Looking at this section now, I think the language in the recommendation could be clarified a bit. While it is indeed based in the YAML terminology of referring to a sequence of documents as a stream, validating a document does require at least some processing of it. Maybe something like this?

Incremental parsing and processing of a YAML stream can produce partial results and later indicate failure to parse the remainder of the stream. To prevent partial processing, implementers might prefer validating and processing all the documents in a stream at the same time.