Telechat Review of draft-ietf-cellar-flac-12
review-ietf-cellar-flac-12-secdir-telechat-sparks-2023-10-12-00
Request | Review of | draft-ietf-cellar-flac |
---|---|---|
Requested revision | No specific revision (document currently at 14) | |
Type | Telechat Review | |
Team | Security Area Directorate (secdir) | |
Deadline | 2023-10-17 | |
Requested | 2023-10-02 | |
Authors | Martijn van Beurden , Andrew Weaver | |
I-D last updated | 2023-10-12 | |
Completed reviews |
Genart Last Call review of -11
by Reese Enghardt
(diff)
Secdir Telechat review of -12 by Robert Sparks (diff) |
|
Assignment | Reviewer | Robert Sparks |
State | Completed | |
Request | Telechat review on draft-ietf-cellar-flac by Security Area Directorate Assigned | |
Posted at | https://mailarchive.ietf.org/arch/msg/secdir/7-LqQkmvnsHTruRKwZ34e3IeIE8 | |
Reviewed revision | 12 (document currently at 14) | |
Result | Has issues | |
Completed | 2023-10-12 |
review-ietf-cellar-flac-12-secdir-telechat-sparks-2023-10-12-00
I have reviewed this document as part of the security directorate's ongoing effort to review all IETF documents being processed by the IESG. These comments were written primarily for the benefit of the security area directors. Document editors and WG chairs should treat these comments just like any other review comments. This document is very thorough and well-written. The security considerations section, in particular, reflects significant effort. It is almost ready for publication a Proposed Standard RFC. There is at least one issue to address before publication, however. ---- Issue: The discussion (and possibly the specification) of the use of URLs in metadata blocks is insufficient. Relying only on explicit user approval to access the resource still leaves many possible exploits. What does explicit approval mean? Is clicking approval once in an application to say "Yes, you can download album cover art" sufficient? Consider an instance, particularly one served over HTTP using the content type registered by this document that becomes virally popular and contains a URL pointing to a victim. Consider again such a thing where the contained URL is, perhaps by mistake, exactly the URL originally retrieved (causing a naive implementation to enter a request loop). Some additional discussion of what is reasonable to retrieve is warranted, and the spirit of same-origin should be taken into account. Privacy should also be mentioned - what sort of tracking can be accomplished through accesses to the URL? There should also be acknowledgment of the risks of allowing arbitrary content to be bundled into FLAC files, particularly in contexts where the MIME type defined in this document is used (web retrieval, multipart/mime in email, wherever else). How might this be used to circumvent content security policies at firewalls, for example? The document already acknowledges that executable code can appear here, but that's only one type of risk, and the discussion in the document steers quickly to the implication that the executable code is not malicious. Is it ever safe to actually execute such code? ---- Minor issue: "mux" is used 5 times in the document (in various forms) without definition, and once normatively: "The offsets MUST NOT be updated on muxing to reflect the new offsets of FLAC frames in a container." ---- Nit issue: This is really almost editorial, but I'm pulling up here so that the SEC ADs see it early; It's a little odd to lean on RFC4732 they way the document does. I understand the intent, but it requires the reader to do some lateral thinking to apply that RFCs guidance to this document. Consider some words motivating how you are intending for it to apply here. ---- More of a question than an issue, but I think the ADs consider the answer: Why isn't this document moving the registry currently at https://xiph.org/flac/id.html to IANA? ---- The rest are editorial comments in document order: Definitions at *Interchannel samples* - you define counts here not samples - maybe the list item should be *Interchannel sample counts* "side-right" and "right-side" appear 2 and 3 times (not counting capitalized uses), respectively, in the document. Are the intended to mean the same thing? Is there a need to have two terms? "does not lead to non-lossless behavior" is a bit of a head-spin. Would "does not lead to lossy behavior" suffice? It would be nice to explicitly say _why_ you allowed a mix of big and little-endian encoding when you note it just after Table 1. Who is the first sentence of section 6 for? Consider deleting it and renaming the section 6 to "An overview of the FLAC format layout" At "FLAC supports up to 127 kinds of metadata blocks", consider removing "up to". It supports exactly 127 kinds of metadata blocks. Consider deleting the one use of "et cetera" - the sentence already says the list isn't exclusive. Consider quoting "fLaC" when your are talking specifically about the marker in prose everywhere (as you quote it in 10.3) What is "pure" overhead? Consider removing the word "pure". At the restriction in section 7 of the maximum block size for 48kHz audio to 4608 - consider showing where 4608 came from. It takes a moment to realize what Table 3 is intended to convey. A short introductory sentence or a table caption would go a long way to help the reader. At 8.3 - it might be nice to note that the n in u(n) here is what's present in the 3 bytes of metadata header block length. Consider replacing the places you use NUL with 0x00 (which you use elsewhere in the document). This makes it more clearer how many 0 bits you really mean. Remember ASCII started out as 7-bits - RFC20 talks about sticking it in an 8 bit byte. I will almost, but not entirely, let "A bright colored fish" pass without comment as it's not this document's responsibility. In 9.1.5 at "non-consecutive" did you mean perhaps "strictly increasing". Consecutive coded frame numbers might make sense, but sample numbers wouldn't. There's a word missing in "There are also audio processors like lossyWAV that enable users to improve compression of their files by a lossless audio codec in a non-lossless way." Perhaps "by using a"? Consider breaking the discussion of bit depth of a subframe out of section 9.2.3 into its own section - it stands (and is used in other parts of the document) for other things than constant subframes. At "This means that decoding MUST be a sequential process" - I think you should say "is necessarily" instead of "MUST be". This really isn't using the 2119 term as intended. In 9.2.6 consider being explicit about whether the coefficients are each represented as 2's complement signed? At section 10, I am suspicious that "very thin transport layer" is not going to communicate what you really mean, but have no immediate suggestions for improvement. In the first sentence of Appendix A - consider "is" rather than "MUST be". At Appendix C.5 - Consider saying "32768" explicitly rather than "last block". (Especially if you didn't _mean_ 32768, but rather the whole last row of Table 14). I don't know what it means to satisfy an interoperability consideration. Please consider rewriting the sentences in C.7 that use the construct. And finally - Please reconsider using the TITLE example in D.2.5 that leaves you using an rtl script inline with the ltr prose. You are within your rights to do so, but it is stabbing at a place where the world (not just our own tooling) isn't as ready as we would like it to be.