Re: nping echo protocol rfc, feedback

["Luis MartinGarcia." <luis.mgarc () gmail com>]

Hi Toni,

Thanks for taking the time to have a look at the RFC and write such
complete report. My replies are interpolated below.

On 03/11/2011 03:21 AM, Toni Ruottu wrote:
> In chapter 2.2 the description of Message Type says "It must be one of
> the type codes defined in section 2.2."
> I think the section number here is wrong.

You are right. It should say "2.3".

> There are some fields in the spec that store timestamps, and after
> reading the whole spec it remains unclear to me which time zone the
> time stamps should be in.

The RFC says that the timestamp is expressed using the epoch time
format, which is defined to be in the UTC zone. However, you are right,
that the text could be more descriptive, so I've just added "UTC".

> For some reason the timestamp field is explained for each message type
> separately. The description seems to be always the same. Some other
> shared fields are only explained once, in the chapter about the
> general message format.

It is only explained in two messages, not in all of them, but I agree
it's redundant so I've removed it.


> In chapter 2.6 the description of Client Nonce field states "Nonce
> value received from the client in the previous
> NEP_HANDSHAKE_CLIENT message." I understood previously that there
> could be only one NEP_HANDSHAKE_CLIENT message sent over one
> connection. At this point I started wondering, if the server should
> use the previous one even when that was received over another
> connection.

Mmm, I'm not sure I'm following you. The initial handshake is a
three-way handshake defined as:

1)  S -> C   NEP_HANDSHAKE_SERVER
2)  C -> S   NEP_HANDSHAKE_CLIENT
3)  S -> C   NEP_HANDSHAKE_FINAL

In the last message, what the server does is echoing the nonce received
from the client in step 2. If the server does not receive a
NEP_HANDSHAKE_CLIENT, it would not produce any NEP_HANDSHAKE_FINAL, so
there is always a client-generated nonce to echo in the
NEP_HANDSHAKE_FINAL message. Also, the server does not keep state after
a NEP session has ended, so two subsequent connections from the same
client are treated as separate connections from different clients.


> The PAYLOAD_MAGIC picture shows a Length field, but the length of the
> Length field is not mentioned anywhere. From the picture I'd assume
> that it is one byte, but I would want to read it from somewhere to be
> sure I understand it correctly.

You are right. I've improved the description of the PAYLOAD_MAGIC spec.


> In chapter 2.8 the description of Packet Length field ends "...must
> not exceed 9212 bytes." I think this should be changed to be more
> explicit whether it is discussing the data or the field. Either "The
> echoed packet must not exceed 9219 bytes." or "The value stored in
> this field should never be bigger than 9212."

I don't really agree with this one. I think the meaning is clear because
in the next paragraph, when the "Packet" field is defined, you can read
"As noted before, the maximum length for an echoed packet is 9212 bytes.
Any packet that exceeds that length must be truncated.". However, there
is no harm in making that more explicit so I've rewritten the
description to be more precise.


> Chapter 2.10 has an example at the of the first paragraph. It starts
> with "eg:" and lists examples, then it ends with "etc". One or the
> other should be dropped. The list is not expected to be complete
> because it is just a list of example, so etc is not needed.
> Alternatively one could drop "eg:" making it a complete list, and
> stating "etc" at the end to cut the list. With "etc" the way the list
> continues should probably be obvious from the example provided, so
> maybe dropping "etc" and keeping "eg" makes more sense. I think eg is
> written "e.g." as it is an acronym for two words, but I am not exactly
> sure about this.

You are right. These kind of things are hard to notice for me, as I'm
not a native english speaker. I know you are not either, but you guys in
northern Europe are always ahead of us, poor southern Europeans ;)


> In the same chapter the explanation of Error Message field explains
> requirements for null characters. I think it would make sense to
> clarify that the last byte of the field needs to be null in all cases.

I don't agree with this one. I thinks that can be trivially inferred
from the description.

> The explanation about receiving encrypted messages states about the
> first step: "Reads 128 bits and decrypts them." What exactly does
> decrypting here mean? Some messages are encrypted partially and some
> are not encrypted at all. Should they still be decrypted somehow?

OK, I've changed the description so it's clear that the process applies
for any message that is fully encrypted (this is, any message other than
NEP_HANDSHAKE_SERVER, NEP_HANDSHAKE_CLIENT or NEP_HANDSHAKE_FINAL).

> step six has a minor language error "higher or equal than" -> "higher
> than or equal to"

Fixed.

> step seven. I stared at the equation for some time before
> understanding what it means. Maybe it could be moved to context in the
> sentence or clarified with (remaining = TotelLength - 128bits)

Done.

> In the following paragraph I would replace "this last statement" with
> the statement itself. The paragraph also states that "all messages are
> encrypted" I think the beginning of the spec stated really explicitly
> that the first message is not encrypted.

I've decided to remove the whole paragraph because I don't even think an
attacker can successfully inject a replayed message in a session. The
attacker can try to replay NEP_HANDSHAKE_SERVER or NEP_HANDSHAKE_CLIENT
messages but the nonces guarantee their freshness. The rest of messages
are sent encrypted, and as the IVs depend on previous messages, any
replayed message would not be decrypted correctly and therefore, its
would have an invalid MAC, etc.


> In 2.13 there is a typo "sesson" -> "session"

Fixed.

> a bit forward "As described above, a total of 5 symmetric keys are
> used." I would change "described" to "mentioned" as the earlier
> description was not too long.

Ok.

> In the key explanations sometimes the concatenation is marked as
> (SERVER_NONCE|CLIENT_NONCE) and sometimes not. I tried really hard to
> figure out if the two cases were different, but I am still not sure. I
> think they are the same. Maybe the notation should be copied to all
> cases or dropped.

You are right. I've replaced the "|" symbol with the plus sign to make
all concatenations consistent.


> Some things seem to be missing from the rfc. It seems that the
> encryption key needs to be truncated before it is used as a key for
> the hmac authentication codes. Openssl did not complain to me when I
> tried to use a longer key. I am not sure, if it would have worked
> correctly. Also, how does the encryptionless mode work? Is it out of
> the scope of this specification? What kind of security risks does this
> service impose on ones system?

Yes, I agree that the RFC could be more complete. I have plans to work
on this in the next few months, so hopefully we'll have better
documentation soon.


> I am also not sure I understood how the encryption IVs and nonces
> should be carried in a long discussion from message to message. I
> might very well be lacking in my theoretical background. I get the
> feeling that this is based on some more general patterns that I am not
> too familiar with. Maybe it can be clarified, but maybe that is not
> necessary.

The nonces are used 1) to guarantee the freshness of a session
establishment, and 2) to be used as material for key derivation. After
all crypto keys have been generated, the nonces are not used anymore.

The IVs are implicit because the IV for message N is the last ciphertext
block of message N-1. This is more or less like if we took all messages
generated by one end, and encrypted them in CBC mode with a single
initial IV, and then sent them as a series of chunks.


> Finally, examples for encryption and hmac would help in debugging
> implementations. Having even one example of the password, nonce and
> typeid, and the key they are supposed to produce would make initial
> debugging a breeze. Not to mention similar examples of the encryption
> and mac. I realize such examples might be too long to be included
> within the text. Maybe they could be included in some sort of
> appendix?

Good idea. I'll keep it in mind.


> An interesting protocol it is. Keep up the good work. --Toni


Thanks again for taking the time to report all this. I've just updated
the RFC with most of your suggestions (SVN rev r22525). Please let me
know if you find any other inconsistencies or problems.


Best regards,


Luis MartinGarcia.

_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://seclists.org/nmap-dev/