Re: [Cellar] Roman Danyliw's Discuss on draft-ietf-cellar-flac-12: (with DISCUSS and COMMENT)

[Martijn van Beurden <[e-mail]>]

Hi Roman,

A new version of this document had just been submitted. I believe it
addresses all issues raised. Many thanks for your thorough review.

Kind regards,

Martijn van Beurden

Op di 17 okt 2023 om 18:21 schreef Roman Danyliw via Datatracker
<[e-mail]>:
>
> Roman Danyliw has entered the following ballot position for
> draft-ietf-cellar-flac-12: Discuss
>
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
>
>
> Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/
> for more information about how to handle DISCUSS and COMMENT positions.
>
>
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-cellar-flac/
>
>
>
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
>
> ** Section 8.4
>    The application metadata block is for use by third-party
>    applications.  The only mandatory field is a 32-bit identifier.  An
>    ID registry is being maintained at https://xiph.org/flac/id.html
>    (https://xiph.org/flac/id.html).
>
> -- Did the WG discuss the consequences of having normative behavior maintained
> on this external website?  I currently see < 30 entries.  Why can’t this be
> maintained by IANA?
>
> -- This third-party application registry URL needs to be a normative reference.
>
> ** Section 8.6.1
>    For a more comprehensive list of possible field names, the list of
>    tags used in the MusicBrainz project (http://picard-
>    docs.musicbrainz.org/en/variables/variables.html) is recommended.
>
> -- It is unclear how this guidance should be treated by implementers.  Is this
> normative behavior to: (a) use the MusicBrainz URL to understand the semantics
> of these field making this reference normative; or (b) this is a free-form
> field outside the scope of this specification but there are community efforts
> like MusicBrainz which is an _informative_ reference.  In either case this URL
> needs to be some kind of formal reference.
>
> -- Is there are reason why there isn’t an IANA registry for these field name?
>
> ** Section 8.7.1  Please make [ISRC-handbook] a normative reference since that
> specification describes the track number value.
>
> ** Section 8.8.
>   the media type and description
>    are prepended with a 4-byte length field instead of being null
>    delimited strings.
>
> What is the format of “media type”?  Is it
> https://www.iana.org/assignments/media-types/media-types.xhtml?  I ask because
> “-->” is later mentioned as an escape sequence.
>
> ** Section 8.8.  The values of table 13 are underspecified:
>
> -- per value = 1, “PNG file icon of 32x32 pixels”, please provide a reference
> to the PNG format
>
> -- per value = 17, “A bright colored fish”, what is that?
>
> ** Section 8.8.  The URI mechanism described in the Picture field of the
> metadata block needs further security considerations.  The SECDIR review also
> pointed this out and discussion on possible new language has started.  This
> guidance needs to explicitly say that:
>
> -- the Security Considerations of RFC3986 apply (to cover threats of
> maliciously craft URLs)
>
> -- following external URLs introduces a tracking risk from on-path observers
> and the operator of the service hosting the URL.  Likewise, the choice of
> scheme, if it isn’t protected like https, could also introduce integrity
> attacks by an on-path observer.
>
> -- a malicious operator of the service hosting the URL can return arbitrary
> content that the parser will read
>
> -- implementers must guard against directory traversal attacks (since relative
> URIs are permitted) and be cognizant of same-origin issues (and localhost and
> local network) even more broadly
>
> -- (per SECDIR review) there could be a DoS attack against the operator of the
> service when the URI from the FLAC file is retrieved
>
> ** Section 10.  Since this document is describing normative behavior on
> embedded this work into another container, that other container needs to be
> named normatively.  Specifically, please make [RFC3533] (Ogg) and
> [I-D.ietf-cellar-matroska] (Matroska) normative references.
>
> ** Section 12
>    FLAC files may contain executable code, although the FLAC format is
>    not designed for it and it is uncommon.  One use case where FLAC is
>    occasionally used to store executable code is when compressing images
>    of mixed mode CDs, which contain both audio and non-audio data, of
>    which the non-audio portion can contain executable code.
>
> Thanks for calling this out.  From this cautionary text, it is not clear to me
> where in the FLAC format this executable code would be insert.  What guidance
> can be given to implementers about recognizing this executable code and
> treating it with care?
>
>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> Thank you to Robert Sparks for the SECDIR review.  Also, thank you to Martijn
> van Beurden for engaging on the feedback.  Some of my DISCUSS feedback aligns
> with this review.
>
> ** Section 7.
>    The flac command-line tool,
>    part of the FLAC reference implementation (see Section 11), generates
>    streamable subset files by default unless the --lax command-line
>    option is used.
>
> -- This single reference to a particular command line option seems out of
> place.  Why mention this one specifically?  Are there other command line
> switches that matter?
>
> -- Section 11 has text to remove upon publication.  Should this text also be
> removed since it won’t make sense without that section?
>
> ** Section 8.2.
>
>    The MD5 signature is made by performing an MD5 transformation on the
>    samples of all channels interleaved, represented in signed, little-
>    endian form. ...
>
> This paragraph describes the need to construct a “MD5 signature”.  I had
> trouble understanding on what a “MD5 transformation” is.  How is it computed?
>
> ** Section 8.8.  Please provide a reference for ID3v3 specification.
>
> ** Section 9.1.5
>    When a frame number is encoded, the value MUST NOT be larger than
>    what fits a value of 31 bits unencoded or 6 bytes encoded.  Please
>    note that as most general purpose UTF-8 encoders and decoders follow
>    [RFC3629], they will not be able to handle these extended codes.
>
> I was confused on why a UTF-8 encoder or decoder would be used to process a raw
> octet stream.  Coded numbers don’t seem to be related to text strings.
>
> ** Section 9.2.2.  Please provide informative references for “Meridian Lossless
> Packing codec” and “lossyWAV”.
>
> ** Section 10.
>    The FLAC format can be used without any container, as it already
>    provides for a very thin transport layer.
>
> It wasn’t clear to me what “thin transport layer” meant here.
>
> ** Section 10.3.  Recommend being clearer that the “full encapsulation
> definition of FLAC audio in MP4 is outside the scope of this document”.
> Perhaps: OLD
>    The full encapsulation definition of FLAC audio in MP4 files was
>    deemed too extensive to include in this document.  A definition
>    document can be found at [FLAC-in-MP4-specification]
> NEW
> The full encapsulation definition of FLAC audio in MP4 files was    deemed too
> extensive and is out of scope for this document.  A possible approach is found
> at [FLAC-in-MP4-specification]
>
> ** Section 12.
>    Parsers MUST employ thorough checks on whether a found coded
>    number or seekpoint is at all possible.
>
> Is this check intended to verify that these seekpoints are in bounds?
>
>
>