Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thinking about changing handshake alert behavior for debug. #606

Open
sbernard31 opened this issue Apr 9, 2018 · 10 comments
Open

Thinking about changing handshake alert behavior for debug. #606

sbernard31 opened this issue Apr 9, 2018 · 10 comments

Comments

@sbernard31
Copy link
Contributor

Currently an unsuccessful handshake because of bad PSK is ignored (server does not answer)
And with #605 unknown psk identity will do the same.
This is the recommended behavior. (referring the TLS and DTLS RFC).

But, it seems there is needs to use scandium as test server to debug clients. So more information could be helpful.
An idea about a way to configure DTLSConnector to change its behavior to send explicit Alert (instead of ignoring record) was proposed.
At first sight, I liked the idea, but I had discussions with teammate and he convinced me this is maybe not a so good idea.

If we expose sandboxes or examples with a default behavior which is no so recommended, we could send a "bad message" to developers, meaning this is the right behavior or the behavior to expect. This could led to new bad DTLS implementation and could affect interoperability.

If the needs is to help developers/debugging maybe, we should made an effort to provide easy/better logs instead.

Currently using : <logger name="org.eclipse.californium.scandium.DTLSConnector" level="DEBUG"/>

I get this kind of logs.
For UNKNOWN PSK IDENTITY :

15:43:58.144 DEBUG [DTLSConnector]: Discarding Handshake (22) record from peer [/127.0.0.1:48546]: Cannot authenticate client, identity [Client_identity!] is unknown (o.e.c.s.DTLSConnector.discardRecord:1781)

For BAD PSK :

15:43:20.670 DEBUG [DTLSConnector]: Discarding Handshake (22) record from peer [/127.0.0.1:59581]: MAC validation failed (o.e.c.s.DTLSConnector.discardRecord:1781)

Changing log level is not so easy for beginners and maybe we could propose a verbose mode (-v arguments) with a default log configuration which helps to debug.

WDYT ?

@sbernard31 sbernard31 changed the title Thinking about changing handshake alert for debug. Thinking about changing handshake alert behavior for debug. Apr 9, 2018
@jimsch
Copy link

jimsch commented Apr 15, 2018

While it is true that the DTLS specification recommends that records which are invalid are silently ignored rather than having a fatal alert generated. In practice there is a big difference between doing this during a handshake and after the finish message has been processed. When invalid records are ignored after the handshake has finished, the client will continue and send new records as appropriate without any issues. At this point in time, sending a fatal alert is going to needlessly shutdown the DTLS connection. Not sending a fatal alert during the handshake, specifically for the finish message, means that the client is going to get stuck in the state of re-sending the same message set over and over without being able to advance until such a time as it decides to stop on its own. (As the client never gets a response to the last pair of handshake records - the CSS and Finish record - It will continually time out and resend the same messages which are treated in the same manner by the server.) Sending the fatal alert for a bad record containing the finish message is going to have better network behavior.

That said, per the authors of TLS/DTLS the behavior as implemented is not wrong. It is merely IMO sub-optimal for the handshake case. I have not verified this, but I have been told that this will not be an issue for DTLS 1.3 as the handshake will fail in a different manner.

@jimsch
Copy link

jimsch commented Apr 16, 2018

Note this is the same issue as was raised in #592

@boaks
Copy link
Contributor

boaks commented Apr 16, 2018

#592 was assuming that the PSK support in scandium was broken, but it shows, that it was only the wrong secret. FMPOV, for that issue #592 the logging message

Discarding Handshake (22) record from peer [/31.133.134.176:55480]: MAC validation failed

should be improved indicating, that a different secret may be the cause of that failure.

The other question seems to be, if scandium should offer a configurable possibility to send ALERTs in that case. FMPOV, RFC6347 is NOT clear about that! Until now, only one author answered @jimsch and @sbernard31 questions at all. I would say, it is at least ambig, which parts of RFC6347 should be applied, I don't see, that the RFC specifies a precedence of 4.1.2.7 over 4.2 and so to silently ignore the invalid record is in my opinion a possible implementation, but not the only valid one. For me it seems to be still useful, to extend scandium to send also alert in that case based on a configuration flag.

@jimsch
Copy link

jimsch commented Apr 16, 2018

One additional reason for returning an error in this case would be to free resources on the server faster. If you return an error here then you can just clean things up instead of waiting until some point in the future. If a client stopped quickly and then sent 100s of attempts to negotiate, you are going to have lots of partially negotiated contexts. I don't know what you are doing to reap old contexts, any answer is wrong for some set of conditions.

@jimsch
Copy link

jimsch commented Apr 16, 2018

I have been reading RFC 4279 section 2 about unknown psk identities. It would appear that you have chosen not to return an unknown_psk_identity based on #605. In my reading the specification says you can return the alert, or continue and then respond with a decryption_error alert at the finish in order to hide identities. If you ignore the decryption error on the finish then that alert would never be sent. I believe that violates the spirit of the RFC

@boaks
Copy link
Contributor

boaks commented Apr 17, 2018

For me it seems to be a question of precedence:
RFC6347, 4.1.2.7 tells to drop the message with wrong mac silently, if you run DTLS on UDP. There are reasons for that, but these reasons don't apply to the handshake FMPOV.
RFC6347, 4.2 specifies the differences of the handshake according RFC5246 (and so RFC4279). Though the drop of the alerts are not mentioned there, it seems to be not fully specified.
I posted my opinion about that ambiguity to the tls list, but it seems that the list focus on something else.

In sum, I believe, that, if there is no clear spec., making it configurable would do it best.

@sbernard31
Copy link
Contributor Author

@jimsch, here is a discussion about this on the TLS ietf mailing list.

In practice there is a big difference between doing this during a handshake and after the finish message has been processed. When invalid records are ignored after the handshake has finished, the client will continue and send new records as appropriate without any issues.

I agree but a client must handle this anyway, as nothing assure that server will not lost DTLS connection later. So client must be able to detect that records sent was ignored and retry a new handshake (or find new credentials)

Sending the fatal alert for a bad record containing the finish message is going to have better network behavior.

Yes, it's probably an optimization but not so frequent use case. I mean this happened only for miss-configured devices or credentials rotations.

One additional reason for returning an error in this case would be to free resources on the server faster.

OK, but we must deal with incomplete handshake anyway.

I have not verified this, but I have been told that this will not be an issue for DTLS 1.3 as the handshake will fail in a different manner.

You talk about optional ACK message ? except that I can see any difference about that ?


My general point of view about "sending an alert" is :

  1. I'm not sure there is not security issues. Personally without re-negociation, I can not see security issue. But when we talk about security you should have good reason to not follow specification.
  2. Eric Rescorla (author of the DTLS spec) says that the spec should be intrepreted as "ignore the message".
  3. The benefits is real but is not so frequent.
  4. Clients and server should handle incomplete handshake and DTLS connection lost anyway, so it's just an optimization.

So, at short-term I would keep "ignore" behavior and improve logs.

If we want to make it configurable, we should get more information from the authors to be sure it's a good idea to send alert.

@jimsch
Copy link

jimsch commented Apr 25, 2018

  1. If there was a security issue, then TLS suffers from what ever that security issue is as it will always send the alert.
  2. I disagree that the spec says MUST ignore the message. The spec says:

In general, DTLS implementations SHOULD silently discard records with
bad MACs or that are otherwise invalid. They MAY log an error. If a
DTLS implementation chooses to generate an alert when it receives a
message with an invalid MAC, it MUST generate a bad_record_mac alert
with level fatal and terminate its connection state.

Thus it is perfectly permissible to fail with an alert if the implementation desires to. Nor does it say that the same behavior is required for all messages.

  1. I have no idea of how frequent the benefits are going to be. The problem is that they are going to be one of diminishing returns. They are going to be huge on the first connect and then negligible after that point.
  2. It is true that both clients and servers need to be able to handle incomplete handshakes. But since this is not an item that is mentioned in the RFC anyplace it is not surprising that the code base that I am working with does not have this implemented correctly.

Improving the logs does not necessarily do me any good if I am testing a server of somebody in Europe while I sit in the US and they left their system up overnight for me. I would not be able to get to the logs to figure out what is going wrong.

@sbernard31
Copy link
Contributor Author

sbernard31 commented Apr 26, 2018

  1. There are big differences between TLS and DTLS. One of them is the transport layer. So this could be a security issue in DTLS and not in TLS ...

  2. Scandium run over UDP and the specification say clearly about sending an alert for invalid record :
    "Note that if DTLS is run over UDP, then any
    implementation which does this will be extremely susceptible to
    denial-of-service (DoS) attacks because UDP forgery is so easy.
    Thus, this practice is NOT RECOMMENDED for such transports.
    "

To be clear, I'm ok sending an alert could be better from a behavior point of view, but I would not sacrifice security for this. So I will try to have more information from IETF.

@boaks
Copy link
Contributor

boaks commented Apr 28, 2018

FMPOV:
Have it configurable would fit best for both.
Discussion about obvious ambiguity in RFCs can take very long time, and so configurability would leave the decision to the server's admin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants