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

From: Barry Leiba <[email-masked]>
Date: Wed, 21 Sep 2016 14:25:57 +0100

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 last call comments.

There are no major issues here, but there are a number of things I
think need to be addressed before this document is ready.

References:
RFC 1951 (DEFLATE) MUST be implemented with this, so it needs to be a
normative reference.

-- Section 2.1 --

   This document defines one
   compression algorithm: DEFLATE.  This algorithm is mandatory to
   implement and MUST be supported in order to advertise the COMPRESS
   extension.

Just to be clear and specific, I suggest changing the second sentence
to say "and MUST be supported and listed in the advertisement of the
COMPRESS extension."

-- Section 2.2.2 --

   In order to help mitigate leaking authentication credentials via for
   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
   command received from an unauthenticated client after a compression
   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.)

   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.)

Same question.

-- Section 2.3 --

At the end of the first example, it would be useful to add another
CAPABILITIES command to show that COMPRESS DEFLATE is no longer
listed.

-- Section 5 --

   This section contains a list of each new response code defined in
   this document and indicates whether it is multi-line, which commands
   can generate it, what arguments it has, and what its meaning is.

This is a total nit, but this sentence seems odd when there's only one
new code (there's also nothing that says whether it is or isn't
multi-line).  I suggest, instead, "The following new response code is
defined in this document:".

-- Section 6 --

   NNTP commands other than AUTHINFO are not believed to divulgate
   confidential information

Qu'est que c'est "divulgate"?  Perhaps you mean "divulge", unless
you're Shakespeare.

   In case confidential articles are accessed in private
   newsgroups, special care is needed: implementations SHOULD NOT
   compress confidential data together with public data when a security
   layer is active, for the same reasons as mentioned above in this
   Section.

Sorry: it is not clear to me what reasons those are; can you be
specific here?  And why SHOULD, and not MUST?

   Additionally, implementations MAY ensure that the contents of two
   distinct confidential articles are not compressed together.

Of course they MAY, but what's the point of saying that.  I think
you'd be much better off skipping the 2119 key word here, and instead
say something about why it might be good to do this: what's the
advantage?  Something like, "Additionally, implementations would do
well to ensure that the contents of two distinct confidential articles
are not compressed together, because [of this advantage]."

   Future extensions to NNTP that define commands conveying confidential
   data SHOULD ensure to state that these confidential data SHOULD NOT
   be compressed together with public data when a security layer is
   active.

I guess this is related the the paragraph I quoted immediately above,
so the question is the same.

-- Section 7.1 --

   Any name that conforms to the syntax of an NNTP compression algorithm
   name (Section 4.3) can be used.

It would be useful here, for IANA's benefit, to specifically highlight
that letters in algorithm names are always upper case.

   However, the name of a registered
   NNTP compression algorithm MUST NOT begin with "X".

   To avoid the risk of a clash with a future registered NNTP
   compression algorithm, the names of private compression algorithms
   SHOULD begin with "X-".

This was discussed in response to another review, and I understand the
"X-" stuff will be removed.  Thanks.

   If it does not implement the compression
   algorithm as specified, it MUST NOT list its registered name in the
   arguments list of the COMPRESS capability label.  In that case, it
   MAY, but SHOULD NOT, provide a private algorithm (not listed, or
   listed with a different name) that implements the compression
   algorithm differently.

Oy.  This really sounds convoluted to me, and the "MAY, but SHOULD
NOT" is a direct contradiction.

I think what you're trying to say here is that if you list a
registered algorithm, it MUST be properly implemented according to the
relevant spec.  Got that.  Now, what about including unregistered
algorithms?  Do you want to say that you MAY include unregistered
algorithms?  Do you want to say that you SHOULD NOT include any?  Why
not, and why might you decide to do that anyway?  What are the
interoperability issues with respect to unregistered algorithms?

   A server MUST NOT send different response codes to COMPRESS in
   response to the availability or use of a private compression
   algorithm.

I don't understand what this is saying at all.  Different from what?
Can you clarify this?

-- 
Barry

---

From: Julien ÉLIE <[email-masked]>
Date: Wed, 21 Sep 2016 17:28:55 +0200

Hi Barry,

> think need to be addressed before this document is ready.

Thanks a lot for your review and your comments.

> RFC 1951 (DEFLATE) MUST be implemented with this, so it needs to be a

OK.

> to say "and MUST be supported and listed in the advertisement of the

OK.

> determination?  (And this is also directly referred to in Section 6.)

I agree that we could change most of SHOULD to MUST here.
The remaining SHOULD would be:

     Consequently, a server
     layer is active

The rationale is because of the wording of RFC 4346, Section 2.1:

    The server MAY list the AUTHINFO capability with no arguments, which
    indicates that it complies with this specification and does not
    permit any authentication commands in its current state.

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.

What wording is preferrable?

>    SHOULD immediately close the connection.  (Then the sending end will

We used SHOULD because RFC 4642 uses SHOULD in a in Section 2.2.2:

    If, after having
    issued the STARTTLS command, the client finds out that some failure
    prevents it from actually starting a TLS handshake, then it SHOULD
    immediately close the connection.
[...]
    If the TLS negotiation fails, both client and server SHOULD
    immediately close the connection.  Note that while continuing the
    NNTP session is theoretically possible, in practice a TLS negotiation
    failure often leaves the session in an indeterminate state;
    therefore, interoperability can not be guaranteed.

I therefore also suggest to keep SHOULD here.
Do you believe we should switch to MUST anyway?  (Wouldn't it be a
too strong constraint?)

> CAPABILITIES command to show that COMPRESS DEFLATE is no longer

Good point.  Besides, I believe I should add it anyway because of
Section 2.1:

    As the COMPRESS command is related to security because it can weaken
    encryption, cached results of CAPABILITIES from a previous session
    MUST NOT be relied on, as per Section 12.6 of [RFC3977].

CAPABILITIES is therefore required to be sent after a successful use
of COMPRESS.

> defined in this document:".

Agreed.  It was just a lame copy/paste from previous RFCs defining
NNTP extensions.

FYI, the core documentation (RFC 3977) uses:

    Response code 100 (multi-line)
       Generated by: HELP
       Meaning: help text follows.

    Response code 111
       Generated by: DATE
       1 argument: yyyymmddhhmmss
       Meaning: server date and time.

Wording suggested for our document:

   This document defines the following new response code.  It is not
   multi-line and has no arguments.

    Response code 206
       Generated by: COMPRESS
       Meaning: compression layer activated

> Qu'est que c'est "divulgate"?  Perhaps you mean "divulge", unless

Je ne suis effectivement ni Shakespeare ni Molière :)

> specific here?  And why SHOULD, and not MUST?

... because the reasons above do not strictly apply to that case.

Suggestion:

     In case confidential articles are accessed in private
     newsgroups, special care is needed: implementations SHOULD NOT
     compress confidential data together with public data when a security
     layer is active.  As a matter of fact, adversaries that observe
     the length of the compressed data might be able to derive
     information about it, when public data (that adversaries know is
     read) and confidential data are compressed in the same compress 
session.

Of course, if you have a more fluent suggestion of wording, you're 
welcome :)

I suggested SHOULD because I thought MUST was too strong.
It would imply that clients and servers have a way to specify
the confidentialness of newsgroups.  It currently does not exist,
and I doubt a user would maintain it in its news client.
That's why I had not used MUST, so as not to declare non-compliant
implementations that do not flag newsgroups as confidential or not.

Am I wrong with that consideration and should we use MUST anyway?

> are not compressed together, because [of this advantage]."

We had in mind the following scenario:

"[...], because adversaries might be able to derive information about
confidential articles as they may have identical header fields (like
Subject, Newsgroups or From) or header fields whose contents may be
predictable (like Xref or Injection-Info)."

Maybe "predictable" is not the right word.
"have a structured pattern"?  "are formatted in a known syntax?"

Examples:
   Injection-Info: news.trigofacile.com; posting-account="eliej";
         posting-host="news.trigofacile.com:2001:41d0:1:6d44::1";
         logging-data="29828"; mail-complaints-to="[email-masked]"
   Xref: news.trigofacile.com trigofacile.test:566

Do you have a preferred wording for that paragraph?
(Should a reference to RFC 5536 be also mentioned?  Like "These header
fields are described in [RFC5536].")

> so the question is the same.

This time, it correctly refers to Section 6 of [RFC3749].
See the beginning of Section 6 of the document:

    Implementers should be aware that combining compression with
    encryption like TLS can sometimes reveal information that would not
    have been revealed without compression, as explained in Section 6 of
    [RFC3749].

If we decide to use MUST instead of SHOULD before in the document, of
course it will need to be homogenized here.

> that letters in algorithm names are always upper case.

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

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".

> "X-" stuff will be removed.  Thanks.

Yep.

> NOT" is a direct contradiction.

Just saying that an implementation has the possibility to do that (MAY)
but is not encouraged to (SHOULD NOT).

> interoperability issues with respect to unregistered algorithms?

To respond your questions:  yes, unregistered algorithms MAY be
included if the implementation wishes to.  Yes, unregistered
algorithms SHOULD NOT be included (bad practice).
Why that?  because out of consistency between RFCs, I'm just declining
in this NNTP extension what Section 3.3.3 of RFC 3977
(core NNTP specification) mentions:

    If the server advertises a capability defined by a registered
    extension, it MUST implement the extension so as to fully conform
    with the specification (for example, it MUST implement all the
    commands that the extension describes as mandatory).  If it does not
    implement the extension as specified, it MUST NOT list the extension
    in the capabilities list under its registered name.  In that case, it
    MAY, but SHOULD NOT, provide a private extension (not listed, or
    listed with a different name) that implements part of the extension
    or implements the commands of the extension with a different meaning.

And I would just say that interoperability issues with that SHOULD NOT
is out of scope of this document.  Implementations SHOULD NOT do that.
A case I see is "private news servers and news clients" (for instance
Giganews [server] and Mimo [client developed specifically for the
Giganews Usenet service]).  They could implement their own unregistered
algorithm, and use it if they wish.  I believe they are clever enough
not to name it LZMA or BZIP2 but XGIGAFEATURE, GIGACOMPRESS...

> I don't understand what this is saying at all.  Different from what?

Suggestion:

     The 206, 403, and 502 response codes a news server answers
     to COMPRESS using a private compression algorithm MUST have the
     same meaning as the one documented in Section 2.2 of this document.

Would it be OK for you with that rewording?

Thanks again for your valuable comments to help improve the quality
of the document.

-- 
Julien ÉLIE

---

From: Barry Leiba <[email-masked]>
Date: Wed, 21 Sep 2016 17:11:34 +0100

Hi, Julien, and thanks for the thorough response.

Further response only on the items where response is needed:

>     no arguments, or not advertise it at all, in response to a CAPABILITIES

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.

> Do you believe we should switch to MUST anyway?  (Wouldn't it be a

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.)

The (very, very small) problem I have with "SHOULD" here is that
"SHOULD" means that you really need to do it unless you have a good
reason not to and have considered the consequences of not doing it.
Along that line, what does it really mean for me *not* to close the
connection when I've received bogus compressed data?

>       Meaning: compression layer activated

Parfait; merci.

> Je ne suis effectivement ni Shakespeare ni Molière :)

He-he...

>     read) and confidential data are compressed in the same compress session.

That sounds great; thanks.

> and I doubt a user would maintain it in its news client.

Yes, that makes sense.

> fields are described in [RFC5536].")

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.

> 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.  Unless, of course, you really want to allow
case-insensitivity here.  If that's so, you'll need to change the
definition of alg-char to use ALPHA, and you'll need to tell IANA that
all values should be converted to upper case when they're registered
(so that we won't wind up with "FLOOB" and "floob" both being
registered).

> but is not encouraged to (SHOULD NOT).

But that's not what it says: "MAY" doesn't mean it's possible; it
means that the choice is entirely an option.  So what that's really
saying is that it's perfectly OK to do either, at your option... and
then making a fairly strong restriction against one of the options.  I
think that's poorly specified and can be confusing.

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?

>     same meaning as the one documented in Section 2.2 of this document.

Ah, yes, very clear now.  Thanks.

> Thanks again for your valuable comments to help improve the quality

You're welcome; I'm glad they've been helpful.

Barry

---

From: Julien ÉLIE <[email-masked]>
Date: Thu, 22 Sep 2016 22:52:20 +0200

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.

-- 
Julien ÉLIE

« Un voyage de mille lieues commence toujours par un premier pas. »
   (Lao Zi)