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

Missing Request/Response Session Check with DTLS Usage on CoAP Server Side #104

Closed
DanielMaier-BSI opened this issue Oct 6, 2016 · 19 comments
Assignees
Labels
Milestone

Comments

@DanielMaier-BSI
Copy link

Californium implements in UdpMatcher.receiveResponse() discard of responses that do not belong to the same DTLS session as the request on CoAP-Client side.

This implements the requirement of CoAP specification:

“The following rules are added for matching a response to a request:
The DTLS session MUST be the same, and the epoch MUST be the same.”

But there is no such such mechanism for this requirement on server side, i.e. a CoAP Server may send a response to a newly established DTLS session that comes from the same IP:Port as the request. To accomplish the RFC it is also mandatory to ensure that no response gets sent to a DTLS session that is not the same as the one of the request.

Example Use Case:

  • There is a CoAP-Server using DTLS with server certificate but without required Client authentication.
  • There is a CoAP-Client client1 with an established DTLS session that observes a resources of this server.
  • A second CoAP-Client client2 initiates a DTLS session using the same IP:Port as client1, e.g. they are behind the same NAT and the NAT devices reuses port mappings after inactivity.
  • Now client2 receives notifies (responses) that were intended for client1. Client2 can decrypt theses notifies because the CoAP-Server encrypts them for the newly created DTLS session. These responses might leak sensitive information.

I tried this out with the given example. But this example is of course not comprehensive. I think this issue also exists when clients do client authentication as long as the client has e.g. a valid PSK.

@sophokles73
Copy link
Contributor

I think that Californium (currently) is exposed to this problem. However, in the LWM2M use case it is usually the CoAP server (the device = LWM2M client) that is behind the NAT, whereas the CoAP client (the LWM2M server) will usually keep the same IP:port.
I therefore think that there is no imminent problem for a LWM2M scenario where the LWM2M clients initiate the DTLS session.

@DanielMaier-BSI
Copy link
Author

I agree with you that this issue is not that critical for LWM2M use case. Only responses of Register, Udpate, deregister are affected.

(Even if someone gets control over LWM2M Server IP, the attacker needs to know the PSKs of clients (in case of using PSKs), Server certificate etc. to initiate new DTLS Session to LWM2M clients)

@sbernard31
Copy link
Contributor

I think this is a general issue. A scandium user is not able to know who we send a message to.
I mean the element connector abstraction just provide a IP/port address as destination.

Main DTLS implementation does not face this issue because, end users manipulate DTLS session. In this case, you can control exactly what you do.

With scandium and element connector, the choice was to abstract this session/connection concept.
If you want to continue in that way, we should probably add a ReceiverIdentity as we already have a SenderIdentity (or more flexible way with a kind of matcher which take a correlation context in parameter but if we do that we need to expose the correlation context more than we do currently)

This information will be used to ensure we send applicationData to the expected destination.

At CoAP level, we should add a way to do the same thing. And we should probably need to know the SenderIdentity for all message (not only for request).

@sophokles73
Copy link
Contributor

sophokles73 commented Oct 27, 2016

I agree with Simon that we would need to introduce more context to the API for sending out CoAP messages. IMHO we should use the CorrelationContext for that purpose and, when sending out messages, check that the session used for communicating with the peer matches (not neessarily identical) the session from the correlation context. We are already using the CorrelationContext when sending messages so it probably will not be a big thing to use it for asserting destination identity based on the DTLS session. Using the peer identity (represented by a Principal as of today) looks problematic to me because we might not even have that identity in cases where the original DTLS handshake has been performed using a cipher without client authentication.

@sbernard31
Copy link
Contributor

Maybe principal should be added to the CorrelationContext. So users could use Principal for correlation is they want.

@sophokles73
Copy link
Contributor

@sbernard31

you seem to disagree with my previous comment where I raised my concern that we might not know the client's identity.

@sbernard31
Copy link
Contributor

I agree with you I think we need to support this case too.

But I understand you want to use a correlation context on send for asserting destination identity.
If I well understand, I set in this context what I want to check when I send a message.
If I want only send through a session for a given principal, I put the principal in the context.
If I have no authentication and I just want to be sure I send the message through a given session, I put this session in the context.

I thought that's what you proposed ^^? Am I wrong ?

@boaks
Copy link
Contributor

boaks commented Nov 16, 2016

We are already using the CorrelationContext when sending messages

Really?

return new RawData(response.getBytes(), response.getDestination(), response.getDestinationPort());

May be I'm wrong, but I see, we use that CorrelationContext to match a received response.
Currently I would like to extent that to match it, when we "really" sent response.
My idea is to store the request CorrelationContext into the RawData of the response, so that a Connector could "match" it immediatly when sending the data. To introduce flexibility in "matching" I would like to add a "CorrelationStrategy".

@sbernard31
Copy link
Contributor

I think you right.

At element-connector level, we already have a CorrelationContext but we use it only for matching received response.

Scandium connector should use it to check that we send message to the right peer. (through the right DTLS Connection)

I like this idea to introduce a CorrelationStrategy.

I'm wondering if we need to add Principal (DTLS identity) in correlation context ?

At CoAP level we probably need to add a way to send API to a specific peer and so adding CorrelationContext to Message ?

@boaks
Copy link
Contributor

boaks commented Nov 16, 2016

I'm wondering if we need to add Principal (DTLS identity) in correlation context ?

At CoAP level we probably need to add a way to send API to a specific peer and so adding CorrelationContext to Message ?

My idea was to do the easy part first:
I hope sending a response/ack is easy, just put the correlationcontext of the request into the RawData
and then check in DtlsConnector.

The second part may be more difficult:
When sending a request, just using the ip-address may fail, therefore we need "peer identity". But I'm not realy clear about that.

(And I have to confess, that mainly I want to see more what we miss to use coap+tcp then to really fix this issue. But though there is an overlapping ...)

@sbernard31
Copy link
Contributor

ok ^^ Let's go for the first part !

@sophokles73
Copy link
Contributor

@sbernard31 , @boaks ,

anybody working on this already? Based on the opinion expressed by Simon on the mailing list, I guess we should add this issue to the 2.0.0 milestone, right?

@boaks
Copy link
Contributor

boaks commented Dec 7, 2016

"Weeks" ago I had a early prototype of the "correlation context strategy" (locally) but I got aware, that this would be too much for on PR together with the introduction of a CorrelationContext for the TCP protocols. Therefore I split the introduction into #148 and I'm currently working on "finalize" that. With some changes in Leshan its quite usable (right now, using the demo certs, also with x509 security).

@sophokles73 sophokles73 added this to the 2.0.0 milestone Dec 7, 2016
@sbernard31
Copy link
Contributor

I'm ok to add this for the 2.0.0 milestones :)

@sbernard31
Copy link
Contributor

Hey guys, what is the current state of this issue ? I would like to move this forward for UDP for the 2.0.0.
@sophokles73 did you start to work on this ?
@boaks should I wait your PRs was merged ?

I'm a bit confuse about tcp ? I thought that this should be moved in a more long term branch as Kai said :
"It also seems like we will need to do some more (bigger) changes in Californium to support observations and clustering over TCP correctly. Personally, I would therefore like to postpone TCP to a later version."
Is it still relevant ?

@boaks
Copy link
Contributor

boaks commented Jan 20, 2017

@sbernard31

I'm a bit confuse about tcp ?

Your right, I stumbled more into TLS and Certificate issues.
I hope, I can close that in the next days and the concentrate on this issue.

boaks pushed a commit to bosch-io/californium that referenced this issue Jan 23, 2017
requests.

Replaces the isResponseRelatedToRequest in the Matcher with
CorrelationStrategy.match(). Intended to be extended with a followup PR
for matching messages on sending to fix issue eclipse-californium#104.

Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
boaks pushed a commit to bosch-io/californium that referenced this issue Jan 24, 2017
requests.

Replaces the isResponseRelatedToRequest in the Matcher with
CorrelationStrategy.match(). Intended to be extended with a followup PR
for matching messages on sending to fix issue eclipse-californium#104.

Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
boaks pushed a commit to bosch-io/californium that referenced this issue Jan 24, 2017
requests.

Replaces the isResponseRelatedToRequest in the Matcher with
CorrelationStrategy.match(). Intended to be extended with a followup PR
for matching messages on sending to fix issue eclipse-californium#104.

Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
boaks pushed a commit to bosch-io/californium that referenced this issue Jan 24, 2017
requests.

Replaces the isResponseRelatedToRequest in the Matcher with
CorrelationStrategy.match(). Intended to be extended with a followup PR
for matching messages on sending to fix issue eclipse-californium#104.

Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
boaks pushed a commit to bosch-io/californium that referenced this issue Jan 24, 2017
requests.

Replaces the isResponseRelatedToRequest in the Matcher with
CorrelationStrategy.match(). Intended to be extended with a followup PR
for matching messages on sending to fix issue eclipse-californium#104.

Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
boaks pushed a commit to bosch-io/californium that referenced this issue Jan 25, 2017
requests.

Replaces the isResponseRelatedToRequest in the Matcher with
CorrelationContextMatcher.isResponseRelatedToRequest(). Intended to be
extended with a followup PR
for matching messages on sending to fix issue eclipse-californium#104.

Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
boaks pushed a commit to bosch-io/californium that referenced this issue Jan 25, 2017
requests.

Replaces the isResponseRelatedToRequest in the Matcher with
CorrelationContextMatcher.isResponseRelatedToRequest(). Intended to be
extended with a followup PR
for matching messages on sending to fix issue eclipse-californium#104.

Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
sophokles73 pushed a commit that referenced this issue Jan 25, 2017
requests.

Replaces the isResponseRelatedToRequest in the Matcher with
CorrelationContextMatcher.isResponseRelatedToRequest(). Intended to be
extended with a followup PR
for matching messages on sending to fix issue #104.

Signed-off-by: Achim Kraus <achim.kraus@bosch-si.com>
@sbernard31
Copy link
Contributor

I think we can close this one.

@boaks
Copy link
Contributor

boaks commented Nov 29, 2017

I had the plan to add some explicit tests about this, but I'm too busy with other stuff ;-(.

@boaks
Copy link
Contributor

boaks commented Jan 18, 2018

Fixed with introduction of endpoint context and endpoint context matcher.
Verified with PR #528

@boaks boaks closed this as completed Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants