Subject: [Curdle] Ben Campbell's Discuss on draft-ietf-curdle-ssh-ext-info-12: (with DISCUSS and COMMENT)

From: Ben Campbell <[email-masked]>
Date: Wed, 13 Sep 2017 12:30:27 -0700

Ben Campbell has entered the following ballot position for
draft-ietf-curdle-ssh-ext-info-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/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-curdle-ssh-ext-info/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

I plan to ballot "yes", but I want to discuss one point first. Alexey mentioned
this in his comments, but I think it's discuss worthy. Hopefully it's an easy
fix, and it may well be because I've missed something obvious. If people think
it really, really needs to be this way, I will clear--but I want to discuss it
first:

- 2.5: The relative order in which extensions appear in an
  EXT_INFO message MUST be ignored by default; but an extension MAY
  specify that the order matters for that extension, in a specific way.

I don't think allowing specific extensions to add ordering requirement works.
It opens up the possibility of incompatible ordering requirements across
extensions. As far as I can tell, the only control over this is the "IETF
Consensus" requirement for adding new extensions. I'm open to arguments that
this is good enough, but my knee-jerk response is that it puts an undue burden
on the consensus process for little return.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Substantive:

-2.3: "This message is sent immediately after SSH_MSG_NEWKEYS, without delay."
That seems to be contradicted in section 2.4, which talks about two other
potential times.

-2.4, last paragraph (asterisk note): "The message MUST be sent at this point
for the following reasons:" That's a confusing use of MUST. Do you mean that
the message MUST NOT be sent unless the reasons are true? Or that if the
reasons are true, the message MUST be sent? Or is this just a statement of
fact, in which case the 2119 keyword is not appropriate?

-2.5, 2nd paragraph: "or it MAY be sufficient that only one party includes it"
That seems like a statement of fact rather than a grant of permission. If so,
the 2119 MAY is not appropriate.

Editorial:

-2.1, first paragraph: "Applications implementing this mechanism MUST add to
the field
  "kex_algorithms", in their KEXINIT packet sent for the first key
  exchange, one of the following indicator names:"

That's hard to parse.  Suggestion:
"Applications implementing this mechanism MUST add one of the
 following indicator names to the "kex_algorithms" field for the first
 key exchange:"

-2.5, 2nd to last paragraph: "... applications MUST
  tolerate any sequence of bytes; including null bytes at any position;
  in an unknown extension’s extension-value."

Redundant to similar normative statement in 2.3.

---

From: denis bider <[email-masked]>
Date: Wed, 13 Sep 2017 22:05:03 -0600

> to add ordering requirement works.

OK, fair point.

That makes two votes against this. There is currently no known extension
that needs this, and if one does arise, there exist other possible
approaches for what is required. I have removed.

> 2.3: "This message is sent immediately after SSH_MSG_NEWKEYS,
> without delay." That seems to be contradicted in section 2.4, which
> talks about two other potential times.

True. I have substantially rewritten sections 2.3. and 2.4. so as to not
conflict, and to make this clearer.

> 2.5, 2nd paragraph: "or it MAY be sufficient that only one party
> includes it" That seems like a statement of fact rather than a grant
> of permission. If so, the 2119 MAY is not appropriate.

True. Changed to lowercase "can".

> -2.1, first paragraph: "Applications implementing this mechanism
> MUST add to the field "kex_algorithms", in their KEXINIT packet
> sent for the first key exchange, one of the following indicator names:"
> That's hard to parse.

I'm guessing you're not a German speaker. :) The issue raised here seems to
be that one needs to read to the end of the sentence to fully understand
what is being added ("one of the following indicator names").

No problem, rearranged sentence.

> -2.5, 2nd to last paragraph: "... applications MUST tolerate
> any sequence of bytes; including null bytes at any position;

Aye, this is redundant on purpose. If this is not followed, it is
devastating to compatibility. OpenSSH in particular has this issue in
currently released versions (including 7.5, the latest). They stated
they'll fix this for 7.6, but a workaround is required to NOT send
extension-values containing null bytes to OpenSSH servers.

The bug is not on purpose, it's because they have multiple functions to
decode an SSH string - some that allow for binary data, some that don't.
They accidentally used the one that doesn't allow binary data to decode
extension-values.

That's bad - and easy to proliferate as long as the most commonly used
extensions don't exercise binary data. Fortunately, OpenSSH doesn't hide
its version string, so the compatibility workaround is straightforward.
That's not the case for all implementations.

This is super important not to miss. The spec is somewhat big, and
implementers might read one part at one time, and another part at another.
I would prefer that this is not missed by anyone.

denis

On Wed, Sep 13, 2017 at 1:30 PM, Ben Campbell <[email-masked]> wrote:

> _______________________________________________
> Curdle mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/curdle
>

---

From: Ben Campbell <[email-masked]>
Date: Wed, 13 Sep 2017 23:22:21 -0500

Hi,

Thanks for the response. Comments inline. I deleted sections that do not seem to need further discussion.

I do not see a response to my comment on the “*” note in section 2.4. Did that change as part of the rewrite of that section that you mention below?

Thanks!

Ben.

> On Sep 13, 2017, at 11:05 PM, denis bider <[email-masked]> wrote:
> 
> That makes two votes against this. There is currently no known extension that needs this, and if one does arise, there exist other possible approaches for what is required. I have removed.

Thanks! I will clear my discuss.

[…]

> I'm guessing you're not a German speaker. :)

Don’t get me started on center embedding in English :-)

> The issue raised here seems to be that one needs to read to the end of the sentence to fully understand what is being added ("one of the following indicator names").
> 
> Aye, this is redundant on purpose. If this is not followed, it is devastating to compatibility. OpenSSH in particular has this issue in currently released versions (including 7.5, the latest). They stated they'll fix this for 7.6, but a workaround is required to NOT send extension-values containing null bytes to OpenSSH servers.
> 
> The bug is not on purpose, it's because they have multiple functions to decode an SSH string - some that allow for binary data, some that don't. They accidentally used the one that doesn't allow binary data to decode extension-values.
> 
> That's bad - and easy to proliferate as long as the most commonly used extensions don't exercise binary data. Fortunately, OpenSSH doesn't hide its version string, so the compatibility workaround is straightforward. That's not the case for all implementations.
> 
> This is super important not to miss. The spec is somewhat big, and implementers might read one part at one time, and another part at another. I would prefer that this is not missed by anyone.

I have no objection to reinforcing a requirement. The issue is redundant normative language, where it becomes ambiguous which piece of text is authoritative. It’s not a real issue here because the statements are consistent (which is why I marked that as “editorial”. ), but it can become a maintenance issue in the future if the resulting RFC is updated or obsoleted.

So my suggestion, which you can freely ignore, is to change one of the occurrences to use descriptive (that is, non 2119)  language.

---

From: denis bider <[email-masked]>
Date: Wed, 13 Sep 2017 22:47:08 -0600

> I do not see a response to my comment on the “*” note in section 2.4.
> Did that change as part of the rewrite of that section that you mention
below?

Aye, sorry for not mentioning that. Most of the text was kept, but it was
slightly rearranged. The sentence containing the inappropriate MUST was
replaced with:

"The timing of the second opportunity is chosen for the following reasons."

> Don’t get me started on center embedding in English :-)

:D

> The issue is redundant normative language, where it becomes
> ambiguous which piece of text is authoritative.

Aye - it does violate the one-definition rule.

I have changed the first mention as follows:

"Implementers' attention is called to Section 2.5., in particular the
requirement to tolerate any sequence of bytes - including null bytes at any
position - in an unknown extension's extension-value."

I have previously contemplated whether to add explicit mention of the
OpenSSH behavior up to 7.5. On the one hand, it is poor practice to
reference individual implementations. But on the other hand, this is a
widespread implementation that everyone will want to interoperate with. If
we provide no guidance about how to handle this bug, it will throw
implementers for a spin, and worse, the compatibility workarounds may not
be ideal. (E.g. people may avoid sending binary extension-values to ANY
version of OpenSSH, instead of just versions up to 7.5.)

For this reason, I have provisionally added the following section under the
"delay-compression" extension:

3.2.3.  Compatibility Note: OpenSSH up to 7.5

  This extension uses a binary extension-value encoding. OpenSSH clients
  up to and including version 7.5 advertise support to receive
  SSH_MSG_EXT_INFO, but disconnect on receipt of an extension-value
  containing null bytes. This is an error fixed in OpenSSH version 7.6.

  Implementations that wish to interoperate with OpenSSH 7.5 and earlier
  are advised to check the remote party's SSH version string, and omit
  this extension if an affected version is detected. Affected versions
  do not implement this extension, so there is no harm in omitting it.
  The extension SHOULD NOT be omitted if the detected OpenSSH version is
  7.6 or higher. This would make it harder for the OpenSSH project to
  implement this extension in a higher version, if they so choose.

This might be unusual, but since the bug exists in current versions, it
will stay relevant for 5+ years. I think it may be worthwhile to ensure
everyone's workarounds are consistent.

The alternative would be to change the spec to disallow binary
extension-values, but I think this is long-term inferior (especially since
OpenSSH has committed to fixing the bug).

denis

On Wed, Sep 13, 2017 at 10:22 PM, Ben Campbell <[email-masked]> wrote:

> I do not see a response to my comment on the “*” note in section 2.4. Did
> that change as part of the rewrite of that section that you mention below?
>
> Thanks!
>
> Ben.
>
>
> > The issue raised here seems to be that one needs to read to the end of
> the sentence to fully understand what is being added ("one of the following
> indicator names").
> >
> I have no objection to reinforcing a requirement. The issue is redundant
> normative language, where it becomes ambiguous which piece of text is
> authoritative. It’s not a real issue here because the statements are
> consistent (which is why I marked that as “editorial”. ), but it can become
> a maintenance issue in the future if the resulting RFC is updated or
> obsoleted.
>
> So my suggestion, which you can freely ignore, is to change one of the
> occurrences to use descriptive (that is, non 2119)  language.
>
> >
> > denis
> >
> >
> >

---

From: Ben Campbell <[email-masked]>
Date: Wed, 13 Sep 2017 23:53:56 -0500

> On Sep 13, 2017, at 11:47 PM, denis bider <[email-masked]> wrote:
> 
> > Did that change as part of the rewrite of that section that you mention below?
> 
> Aye, sorry for not mentioning that. Most of the text was kept, but it was slightly rearranged. The sentence containing the inappropriate MUST was replaced with:
> 
> "The timing of the second opportunity is chosen for the following reasons.”

That works for me, thanks!

[…]

> "Implementers' attention is called to Section 2.5., in particular the requirement to tolerate any sequence of bytes - including null bytes at any position - in an unknown extension's extension-value.”
> 

Thanks!

> I have previously contemplated whether to add explicit mention of the OpenSSH behavior up to 7.5. On the one hand, it is poor practice to reference individual implementations. But on the other hand, this is a widespread implementation that everyone will want to interoperate with. If we provide no guidance about how to handle this bug, it will throw implementers for a spin, and worse, the compatibility workarounds may not be ideal. (E.g. people may avoid sending binary extension-values to ANY version of OpenSSH, instead of just versions up to 7.5.)
> 
> For this reason, I have provisionally added the following section under the "delay-compression" extension:
> 
> 
> This might be unusual, but since the bug exists in current versions, it will stay relevant for 5+ years. I think it may be worthwhile to ensure everyone's workarounds are consistent.
> 
> The alternative would be to change the spec to disallow binary extension-values, but I think this is long-term inferior (especially since OpenSSH has committed to fixing the bug).

I don’t have a strong opinion here. One approach might be to say an extension MUST NOT add binary values, but implementations MUST NOT fail if one is present for an extension you don’t understand. In any case, I will leave that to you and Ekr to decide.

[…]

---

From: denis bider <[email-masked]>
Date: Wed, 13 Sep 2017 23:41:20 -0600

> One approach might be to say an extension MUST NOT add binary values,
> but implementations MUST NOT fail if one is present for an extension you
> don’t understand.

Unfortunately, prohibiting extensions from having binary extension-values
would have significant ramifications that would look very unsightly in the
SSH protocol.

For example, for something like the "delay-compression" extension that
needs to encode two name-lists, if extension-value cannot be binary, then
it would have to be Base64-encoded. This would be a very questionable
choice in a protocol that is otherwise binary and has no reason to
pessimize itself this way, other than that one particular implementation,
at one point in time, used the wrong function to decode a string. :)

On Wed, Sep 13, 2017 at 10:53 PM, Ben Campbell <[email-masked]> wrote:

> > Aye, sorry for not mentioning that. Most of the text was kept, but it
> was slightly rearranged. The sentence containing the inappropriate MUST was
> replaced with:
> >
> position - in an unknown extension's extension-value.”
> >
>
> Thanks!
>
>
> I don’t have a strong opinion here. One approach might be to say an
> extension MUST NOT add binary values, but implementations MUST NOT fail if
> one is present for an extension you don’t understand. In any case, I will
> leave that to you and Ekr to decide.
>
> […]
>

---

From: Ben Campbell <[email-masked]>
Date: Thu, 14 Sep 2017 00:45:16 -0500

> On Sep 14, 2017, at 12:41 AM, denis bider <[email-masked]> wrote:
> 
> Unfortunately, prohibiting extensions from having binary extension-values would have significant ramifications that would look very unsightly in the SSH protocol.
> 
> For example, for something like the "delay-compression" extension that needs to encode two name-lists, if extension-value cannot be binary, then it would have to be Base64-encoded. This would be a very questionable choice in a protocol that is otherwise binary and has no reason to pessimize itself this way, other than that one particular implementation, at one point in time, used the wrong function to decode a string. :)

That’s convincing. I will leave this up to the SSH experts to figure out :-)

Ben.