Subject: Re: [secdir] Secdir review of draft-murchison-nntp-compress-05

From: Julien ÉLIE <[email-masked]>
Date: Mon, 3 Oct 2016 22:47:22 +0200

Hi Barry,

I'm currently reviewing the comments I need to take into account for the 
document.
Do you believe I should add a note about a possible security issue as 
far as the use of DEFLATE is concerned?  (see below)
(An "out of memory" attack?)

Thanks,

-- 
Julien

Le 22/09/2016 à 22:52, Julien ÉLIE a écrit :
> Hi Barry,
>
> Thanks for your answer.  I'll have a deeper look at it soon.
>
> As for the remaining "small points" of rewording (SHOULD, MUST, etc.),
> I'll calmly see them when updating the draft after Last Call.  In case I
> have a doubt for the best wording, I'll ask Ken (co-author) and Michael
> (write-up shepherd) their preference.  We'll obviously manage to come up
> with the "final" rewording as we'll have 3 votes for 2 choices.
>
>
> Now that I think about it, wouldn't it be worth adding something about
> possible "out of memory" attacks in the Security Considerations Section?
>
> According to <http://zlib.net/zlib_tech.html>, "a 50MB file filled with
> zeros [...] a version of run-length encoding optimized for this sort of
> unusual data file [...] could encode the test file in five bytes. That
> would be a compression factor of 10,000,000:1".
> So sending just 5 bytes may require an expansion to 50MB.
>
> Implementations therefore really need to check that the uncompressed
> data does not exceed an upper bound limit.
>
> RFC 1951 (DEFLATE) does not mention it in its Security Considerations,
> and I don't remember having seen that in other RFCs too.  Maybe we
> should say a word for it in the NNTP COMPRESS extension.
>

---

From: Barry Leiba <[email-masked]>
Date: Wed, 5 Oct 2016 16:52:43 -0400

I've been thinking on this since you mentioned it, and I have mixed
feelings.  So let me spread them out here:

On the one hand, it's well known that poorly implemented compression
has a lot of potential for problems of various kinds, including
security.  You're basically considering saying that, and saying that
it's therefore important to implement the algorithms correctly.  Well,
yes.

On the other hand, there *is* this particular issue that we know can
be a sticky point, so... why not mention it?  Kind of saying that you
have to implement the algorithms correctly, and that you should be
particularly careful about buffer sizes when deflating because just a
few bytes can deflate into unexpectedly huge results, and that's often
used to attempt attacks.

And on the third hand (I'm Martian), text is cheap, and if a few words
can be helpful, that seems reasonable.

On the fourth hand (and that's all that Martians have), it's not this
document's job to cover security (or other) issues in the compression
algorithms themselves.

So, yes: in the end, I think it's worth putting in a few words about
this, just to be clear that it's a common trap.

Barry

On Mon, Oct 3, 2016 at 4:47 PM, Julien ÉLIE <[email-masked]> wrote:
> Do you believe I should add a note about a possible security issue as far as
> the use of DEFLATE is concerned?  (see below)

---

From: Julien ÉLIE <[email-masked]>
Date: Sun, 23 Oct 2016 23:00:06 +0200

Hi Barry,

I've just taken into account your review in a new revision of the draft. 
  Thanks again for it!

FYI, the related changes are:
 
https://github.com/ksmurchison/drafts/commit/ea2ad20efd248fd83ec8e859cf2ab0064b3c3b64

Do not hesitate to tell me in case something is still wrong.

>> Maybe you would prefer for our document:
>>
>>     Consequently, a server MAY list the AUTHINFO capability with
>>     no arguments, or not advertise it at all, in response to a CAPABILITIES
>>     command received from an unauthenticated client after a compression
>>     layer is active
>>
>> This way, we would no longer use SHOULD, but MUST and one MAY.
>
> It's a small point, but I don't like "MAY" to introduce two
> alternatives, because it's not clear whether there might be other
> alternatives as well... or not.  How does it work for you to make it
> "MUST" instead of "MAY", saying, therefore, that the server MUST
> either list AUTHINFO with no arguments or not advertise it at all?
> Then it's clear that there are two alternatives, and either is OK.
> No?

OK.  I've adopted your suggestion.

    In order to help mitigate leaking authentication credentials via for
    instance a CRIME attack, authentication MUST NOT be attempted when a
    compression layer is active.  Consequently, a server MUST either list
    the AUTHINFO capability with no arguments or not advertise it at
    all, in response to a CAPABILITIES command received from an
    unauthenticated client after a compression layer is active [...]

Hmm...  I think I should now do a pass on the document and explicitly 
say when "compression layer" only means the one negotiated with COMPRESS.
As a matter of fact, I do not think it's a good idea to say in this 
draft that authentication MUST NOT be attempted when TLS-level 
compression is active!  It would otherwise be a change in how 
authentication works (RFC 4643 heavily mentions the preferred use of 
AUTHINFO along with TLS, and RFC 4642 allows TLS-level compression).
This document would otherwise be an update to RFC 4643, by no longer 
allowing AUTHINFO when TLS-level compression is active.

>>    When compression is active and either the client or the server
>>    receives invalid or corrupted compressed data, the receiving end
>>    SHOULD immediately close the connection.  (Then the sending end will
>>    in turn do the same.)
>
> Well (and we're now down to nit level, so do as you think best here),
> as I see it, it's not a question of level of constraint -- if it were,
> you could say it without 2119 key words, as "the receiving end
> immediately closes the connection, in response to which the sending
> end will do the same."  (Now that I think about it more, I think
> that's the best answer, but if you disagree that's fine.)

You're right.  Your suggestion is also adopted.

    When compression is active and either the client or the server
    receives invalid or corrupted compressed data, the receiving end
    immediately closes the connection, in response to which the sending
    end will do the same.

>>    Additionally, implementations MAY ensure that the contents of two
>>    distinct confidential articles are not compressed together.
>
> As I say, maybe just drop the key word and say it without capitals.
> If it's not easy to explain or determine, maybe just say that it's
> good to do it if you can:
>
> NEW
>    Additionally, it is preferable not to compress the contents of two
>    distinct confidential articles together if it can be avoided.
> END
>
> But, again, it's a small point that probably doesn't need further discussion.

Thanks for your suggestion of wording.  Also adopted!

    Additionally, it is preferable not to compress the contents of two
    distinct confidential articles together if it can be avoided, as
    adversaries might be able to derive information about them (for
    instance if they have a few header fields or body lines in common).
    This can be achieved for instance with DEFLATE by clearing the
    compression dictionary each time a confidential article is sent.
    More complex implementations are of course possible, and encouraged.

>> In the registry, it is indeed best to use upper case.  I can mention it.

Now done, for IANA's benefit:

    Any name that conforms to the syntax of an NNTP compression algorithm
    name (Section 5.3) can be used.  Especially, NNTP compression
    algorithms are named by strings, from 1 to 20 characters in length,
    consisting of upper-case letters, digits, hyphens, and/or
    underscores.

>> Just to be certain of the meaning of your remark:  the algorithm name
>> in the NNTP command is not required to be upper case.
>>
>>      algorithm = "DEFLATE" / 1*20alg-char
>>      alg-char = UPPER / DIGIT / "-" / "_"
>>
>> ABNF syntax is not case-sensitive, so "COmprESs dEFlaTe" is a valid
>> NNTP command, that has the same effect as "COMPRESS DEFLATE".
>
> Actually, that's an ABNF error that I missed: "DEFLATE" doesn't have
> to be in upper case, but any *other* algorithm does (because
> "alg-char" can only contain UPPER, not ALPHA or LOWER).  To fix that,
> I suggest using this:
>
>    algorithm = %s"DEFLATE" / 1*20alg-char
>
> ...and adding a normative reference to RFC 7405... that forces DEFLATE
> to be upper case.

Good catch.  Also done.

    This section describes the formal syntax of the COMPRESS extension
    using ABNF [RFC7405] [RFC5234].

    [...]

      algorithm = %s"DEFLATE" / 1*20alg-char  ; case-sensitive
      alg-char = UPPER / DIGIT / "-" / "_"

> Unless, of course, you really want to allow case-insensitivity here.

Case-sensitivity is better, and we'll keep that.
It is also coherent with SASL mechanisms (case-sensitive), and 
consistent with RFC 3977:

Section 3.1

    Commands in NNTP MUST consist of a keyword, which MAY be followed
    by one or more arguments.

[...]

    Commands may have variants; if so, they use a second keyword
    immediately after the first to indicate which variant is required.
    The only such commands in this specification are LIST and MODE.

[...]

    Keywords are case insensitive; the case of keywords for commands MUST
    be ignored by the server.  Command and response arguments are case or
    language specific only when stated, either in this document or in
    other relevant specifications.

In the case of COMPRESS:
- the command (COMPRESS) is case insensitive (like any keyword),
- the command has an argument (e.g., DEFLATE), that is not a keyword, 
and therefore can be said to be case-sensitive.

> Considering what you say in your response after that, let me suggest
> an alternative:
>
> NEW
>    If the server advertises a registered NNTP compression algorithm, it
>    MUST implement the compression algorithm so as to fully conform with
>    its related specification.  If it does not implement the compression
>    algorithm as specified, the implementation is considered a private
>    algorithm and MUST NOT be listed with the registered name in the
>    arguments list of the COMPRESS capability label.
>
>    Private algorithms with unregistered names are allowed, but SHOULD
>    NOT be used because it is difficult to achieve interoperability with them.
> END
>
> How's that?

All right.  I kept your second paragraph.
I removed the first one because on second thoughts, we don't really need 
saying it.  As Alexey commented in his review, "this sounds like 
motherhood and apple pie".

Thanks again, and have a nice week,

-- 
Julien ÉLIE

« – Tu parles ?
   – Tu parles ! » (Astérix)

---

From: Julien ÉLIE <[email-masked]>
Date: Mon, 24 Oct 2016 22:02:46 +0200

Hi Barry,

[...]
> And on the third hand (I'm Martian)
> On the fourth hand (and that's all that Martians have)

I like these two introductions of alternatives :)

> this, just to be clear that it's a common trap.

OK.
FYI, I've added the following paragraph at the end of the Security 
Considerations Section.  The wording is inspired by the one in RFC 3749.

    Last but not least, careful consideration should be given to
    protections against implementation errors that introduce security
    risks with regards to compression algorithms.  See for instance the
    part of Section 6 of [RFC3749] about compression algorithms that can
    occasionally expand, rather than compress, input data.

-- 
Julien ÉLIE

« Aut bibas aut abeas. »

---

From: Julien ÉLIE <[email-masked]>
Date: Mon, 24 Oct 2016 22:20:46 +0200

Hi Barry,

Thanks again for your valuable comments on the document.  They were very
much appreciated, and permitted to fix a few issues.

I've just finalized a revised draft, taking into account all the comments
received during Last Call.
I just want to highlight the following change in wording.
Ken and Michael, as respectively co-author and document shepherd, please
tell if you think the new wording is not the right thing to do.

Personally, I think this document (draft-murchison-nntp-compress) should
only focus on standardizing the COMPRESS command and not try to fix
how authentication works in another kind of compression (TLS-level
compression).  It would otherwise be an update to RFC 4643.

Updating TLS usage with NNTP is the aim of a second, separate document
(draft-elie-nntp-tls-recommendations) that updates RFC 4643 with
best current practices.  That one discourages the use of TLS-level
compression, thus dealing with authentication layered with a TLS-level
compression method.

>>   instance a CRIME attack [CRIME], authentication SHOULD NOT be
>>   attempted when a compression layer is active.  Consequently, a server
>>   SHOULD NOT return any arguments with the AUTHINFO capability label
>>   (or SHOULD NOT advertise it at all) in response to a CAPABILITIES
>>   layer is active, and such a client SHOULD NOT attempt to utilize any
>>   AUTHINFO [RFC4643] commands.  It implies that a server SHOULD reply
>>   with a 502 response code if a syntactically valid AUTHINFO command is
>>   received while a compression layer is already active.
>>
>> Why are these SHOULD, and not MUST?  Under what conditions would it be
>> necessary or reasonable for an implementation not to abide by these,
>> and what considerations need to be considered when making that
>> determination?  (And this is also directly referred to in Section 6.)
[...]
[...]
> allowing AUTHINFO when TLS-level compression is active.

Pass done.  I updated the wording in a few parts of the document.
The above quoted paragraph becomes:

   In order to help mitigate leaking authentication credentials via for
   instance a CRIME attack [CRIME], authentication MUST NOT be attempted
   after a successful use of the COMPRESS command.  Consequently, a
   server MUST either list the AUTHINFO capability with no arguments or
   not advertise it at all, in response to a CAPABILITIES command
   received from an unauthenticated client after a successful use of the
   COMPRESS command, and such a client MUST NOT attempt to utilize any
   AUTHINFO [RFC4643] commands.  It implies that a server MUST reply
   with a 502 response code if a syntactically valid AUTHINFO command is
   received after a successful use of the COMPRESS command.  (Note that
   this specification does not change the behaviour of AUTHINFO as
   described in [RFC4643] independently of TLS-level compression.
   Authentication is therefore still allowed, even though TLS-level
   compression is active.)

I hope you're all fine with that.
Have a nice day,

-- 
Julien ÉLIE

« Aut bibas aut abeas. »

---

From: Ken Murchison <[email-masked]>
Date: Tue, 25 Oct 2016 07:55:15 -0400

I'm fine with the changes.

-- 
Kenneth Murchison
Principal Systems Software Engineer
Carnegie Mellon University

---

From: Michael Bäuerle <[email-masked]>
Date: Tue, 25 Oct 2016 18:41:26 +0200

> I hope you're all fine with that.

I'm fine with it.

Regards,

Michael Bäuerle