Re: [secdir] secdir review of draft-ietf-json-text-sequence-11

[Nico Williams <[e-mail]>]

On Sat, Dec 13, 2014 at 01:25:33PM -0500, Carl Wallace wrote:
> I have reviewed this document as part of the security directorate’s
> ongoing effort to review all IETF documents being processed by the IESG.

Thanks.

> This document describes a means of encoding and parsing arbitrarily large
> sequences of JSON texts. The document claims no new security
> considerations beyond those in RFC 7159.  I have some concern that the

Though it turns out that RFC7159's security considerations section was
probably too brief.  Anyways.

> addition of a LF by the JSON text sequence encoder that is not removed by
> the JSON text sequence parser has the potential to break detached JSON web
> signatures that cover JSON text sequence elements.  A few general comments
> and questions are below:

That's a good point.  Signing individual JSON text sequence elements is
outside the scope of this document.  Either a complete sequence is
signed, or individual texts are signed with a wrapper.  I wouldn't
recommend transferring a sequence of texts and a parallel sequence of
signatures, say.

> - In the abstract of version 11 the hex value for ASCII line feeds was
> changed to 0x1E from 0x0A.

Yeah, a widely noted typo :/

> - In section 2.1, shouldn’t possible-JSON be defined as *(not-RS) to allow
> for parsing of <RS><RS><RS>?

The preceding 1*RS in JSON-sequence takes care of that, although that
assumes greedy ABNF matching (RFC5234 does not speak to greediness).

> - Section 2.3 should clarify that malformed JSON text sequences are also
> not fatal (I.e., arriving at an RS without having seen a LF).

That's covered in detail in section 2.4.  I wouldn't want to replicate
any part of 2.4 in 2.3.

> - The second paragraph of Section 2.3 suggests that an incremental JSON
> parser may be used across content from multiple sequence elements.  The
> ABNF for encoders does not seem to support this.

Are you thinking of a a sequence like <RS> <JSON-text-w/o-trailing-LF>
<JSON-text> ... being parsed as two JSON texts?

Or are you thinking of a <RS> <JSON-text-containing-RS> <LF>?

The latter must yield a parse error from the JSON text parser.

As to the former... I think I'd thought about it and left it as-is,
though I can't recall why, with the security considerations text about
smuggling being sufficient (except it's not quite).

How about this new text:

   After reading and parsing a JSON text, the sequence parser MUST skip
   to the RS byte following the JSON text (or terminate if at end of
   input).

And in section 3:

   JSON text sequences like <RS> <JSON-text> <JSON-text> <LF> are
   sequences of one text, not two.  JSON text sequence parser
   implementations may emit the first JSON text, but should detect and
   log such sequences.

> - The last sentence of the second paragraph in Section 2.4 is a bit
> confusing. If a JSON-text has no trailing ws and the LF from the JSON text
> sequence is used to match the ws rule, should this be reported as a
> malformed JSON text sequence, a malformed JSON-text or neither?

Neither.  The LF doubles as the ws that makes the number/boolean/null
parse correctly, and as the LF needed for the JSON text sequence.

This is also true for string/array/object, but since those are
self-delimited the presence of the LF is less critical.

Section 2.4 is really

a) justification for the LF,

b) a warning to implementors not to include a bug where permissive
   parsing (not insisting on the trailing LF) results in very incorrect
   results when the top-level value type is a number (or boolean, or
   null).

> - In section 2.4, the “parsers” referenced in the second and third
> paragraphs should be qualified as JSON text sequence parsers or JSON
> parsers.  

When used without a qualifier the intended meaning is "JSON text
sequence parser", but that's a mouthful.  I may add text in section 1.1
about this.

> - In section 2.4, assuming the parser is a JSON text sequence parser, why
> is the MUST drop in the third paragraph necessary given potential use of
> incremental parsers mentioned in section 2.3 and later in 2.4? How do you
> distinguish between a “non-self-delimited top-level value” and a piece of
> JSON-text being parsed incrementally?

See above.  In my implementation I check that the last byte consumed by
the [incremental] JSON text parser was a whitespace character.

> - The examples for possibly truncated JSON texts seem to assume that the
> LF appended by a JSON text sequence encoder is absent. How would a
> truncated JSON text within a properly delimited JSON text sequence be
> detected? [...]

Well, if the LF is present then it's not truncated.  E.g., the number in
<RS>1<RS> may have been truncated, but the number in <RS>123<LF> cannot
have been.

>     [...] For example, if a component feeding JSON text elements to a JSON
> text sequence encoder failed where the JSON text sequence encoder
> succeeds, the result would be a truncated JSON text within a valid JSON
> text sequence. I realize the ABNF should preclude encoding invalid JSON
> texts but there is no text that instructs encoders to fail when invalid
> JSON texts are detected.

Yes, a JSON text sequence encoder should also encode the texts
themselves.  When it doesn't the result may be garbage, but the JSON
text sequence parser can cope with such garbage.

> - The failure to address the LF in the parser section seems like it could
> cause problems for re-encoding.  What would prevent accumulation of LF
> characters as a JSON text sequence was encoded, parsed, re-encoded,
> re-parsed and re-encoded?

There's no requirement that parsing and re-encoding be the identity
function.  There's no such requirement for JSON texts, therefore there
can't be for JSON text sequences.

This comes up a lot for JSON implementations.  Users often want the
result of parsing and re-encoding to leave numbers as-is, object [key]
names in the same order as in the input, indentation/whitespace the
same, and so on.  The answer is generally "sorry, that's ETOOHARD".

(There are implementations that preserve object name/value insertion/
parse order.  But keeping track of encoded number form along with parsed
numbers can be a headache.  Besides, implementations often lose number
precision/range.)

Nico
--