session.h: Modify session_t for RIOT

[leandrolanzieri]

As described in RIOT-OS/RIOT#17849, having port and addr as separate fields in the session_t was causing in certain scenarios to include random bytes (possible due to padding) to the hash calculation for the cookie. This would in turn cause the server to generate inconsistent cookies, thus failing to perform the handshake.

This PR joins the port and the IPv6 address into a single member, which is used to calculate the size.

[boaks]

In order to contribute to a Eclipse project, you need to sign a ECA, see CONTRIBUTING.

[boaks]

Let me propose to change the order of the fields to:

 struct {
    unsigned short port;
    ipv6_addr_t addr;
  } addr;

that's common in the posix definitions.
I'm not sure, if RIOT is only considering IPv6. If not, and also IPv4 cases may be considered, then the test

#define _dtls_address_equals_impl(A,B)                          \
  ((A)->size == (B)->size                                       \
   && (A)->addr.port == (B)->addr.port                                    \
   && ipv6_addr_equal(&((A)->addr.addr),&((B)->addr.addr))                \
   && (A)->ifindex == (B)->ifindex)  

should consider the size when comparing the addr.addr.
I would also like to propse, that the inner field gets then name different from the outer, e.g.

 struct {
    unsigned short port;
    ipv6_addr_t addr6;
  } addr;

Finally, I think to prevent the root-cause from happens in other ports (contiki-ng), the documentation of the size and addr fields should be amended.

[boaks]

@leandrolanzieri

Do you go to sign the ECA?
If not, I may prepare a different PR with my comments.

[leandrolanzieri]

@leandrolanzieri

Do you go to sign the ECA? If not, I may prepare a different PR with my comments.

I will, I'm just having some issue to validate my email address for some reason...

I'm not sure, if RIOT is only considering IPv6. If not, and also IPv4 cases may be considered, then the test

Yes, as a matter of fact RIOT-OS/RIOT#17765 has been merged recently, adding IPv4. I'll update this PR with your comments and also with IPv4 support.

[HendrikVE]

@leandrolanzieri I already have a PR open for IPv4: #129 ;)

[leandrolanzieri]

@leandrolanzieri I already have an PR open for IPv4: #129 ;)

Ok, then no need for this one as yours is already including my changes

[leandrolanzieri]

Ah I did not see that #129 included this as a dependency, then I'll just apply the comment from @boaks

[boaks]

I will,

Great to read.

I'm just having some issue to validate my email address for some reason...

You may need a line as

Signed-off-by: Leandro Lanzieri <leandro.lanzieri@haw-hamburg.de>

as the last in the comment of your commit.

(You must use the same e-mail address used for the ECA).

(I have checked it, currently there is no ECA for "leandro.lanzieri@haw-hamburg.de".)

[leandrolanzieri]

as the last in the comment of your commit.

(You must use the same e-mail address used for the ECA).

(I have checked it, currently there is no ECA for "leandro.lanzieri@haw-hamburg.de".)

Yeah, I did it once with another email, which is also on my GH account (lanzierileandro@gmail.com). Is there a way to add a second address?

[boaks]

Is there a way to add a second address?

Not sure. Isn't it easier to use then the e-mail address of the ECA for the commit?

(By the way, I was able the check the ECA for lanzierileandro@gmail.com, it's valid.)

[leandrolanzieri]

I created a new account with leandro.lanzieri@haw-hamburg.de email address and added the signed-off line to both commits

[boaks]

That legal validation stuff is somehow tricky. And doesn't like twins ;-)

Currently the page of the validation shows "leandrolanzieri" as user. Is that your previous or your new account?

(I was able to check the ECA for the leandro.lanzieri@haw-hamburg.de and it's valid now.)

[leandrolanzieri]

Currently the page of the validation shows "leandrolanzieri" as user. Is that your previous or your new account?

That's the new username

[leandrolanzieri]

(I was able to check the ECA for the leandro.lanzieri@haw-hamburg.de and it's valid now.)

Hmm is there something wrong with the commit message then?

[boaks]

Not sure, why the validation fails.

Anyway, my comment for more documentation about the size and addr was ment in more generic way, contiki-ng was just a reminder.

I would consider, to prevent others, maybe future ports, from that pitfall, to add a general commet ahead of the #ifdefs with the guidance to size and addr (including, that the port is considered to be part of the addr) and explain, that the first size bytes of the addr are considered for the cookie hash and to identify the peer's dtls context.

[boaks]

Hmm is there something wrong with the commit message then?

I guess, not. I can't see something wrong. You may add a "." at the end, but I'm not sure, if that changes anything.
I'm no committer in tinydtls, so it's up to them, how to proceed.

[boaks]

@sbernard31

Just in the case, you can spent some minutes into the ECA verification stuff.
Do you see the failure? Or do you have experience with two users and two e-mail addresses?

[leandrolanzieri]

I would consider, to prevent others, maybe future ports, from that pitfall, to add a general commet ahead of the #ifdefs with the guidance to size and addr (including, that the port is considered to be part of the addr) and explain, that the first size bytes of the addr are considered for the cookie hash and to identify the peer's dtls context.

Ok I will add the documentation before then. But this raises a question: dtls_session_init initializes session_t::size to sizeof(sess->addr), but this would not consider the port for contiki for example, unless set manually somewhere else. Should we rather force addr to always contain the port?

https://github.com/eclipse/tinydtls/blob/706888256c3e03d9fcf1ec37bb1dd6499213be3c/session.c#L62-L67

[boaks]

Yes, good question.

dtls_new_session overwrites that for POSIX by

sess->size = addrlen;

I guess, we need some guidance by @obgm .
I would consider, that the size must be initialized depending on the "platform".

[sbernard31]

session.h: Modify session_t for RIOT #127

About this project

Eclipse Foundation Project:
Eclipse Foundation Specification Project:
Source code: [eclipse/tinydtls](https://www.github.com/eclipse/tinydtls/pull/127)
Pull request: [#127](https://www.github.com/eclipse/tinydtls/pull/127)

Failures

[Leandro Lanzieri](https://accounts.eclipse.org/users/leandrolanzieri) (leandro.lanzieri@****.de)

Tips

Could not validate the users ECA sign-off status. 
Please ensure that the e-mail address of all contributing accounts match a valid Eclipse Foundation account 
with a valid ECA.

Additional information on contributing to Eclipse projects can be found on the Eclipse wiki under Development Resources/Handling Git Contributions, or under Development Resources/Contributing via Git for new committers.

I try to Revalidate it but still get the same issue.

I double check that :

• all commits use same address (in Signed-off commit message and as commit author) ✔️
• the address used has a valid ECA attached ✔️

So from my point of view, this checks failure should not prevent a committer to merge this PR. (This is probably just a eclipsefdn/eca tool bug). (I mean this can just be ignored)

Note also that I think (not sure) that "Signed-off-by" is no more mandatory, the commit author is enough now.

[boaks]

@sbernard31

Thanks for double checking it. I also tried "revalidate" and it didn't work (I got also some "The answer you entered for the CAPTCHA was not correct." without such a CAPTCHA at all.)

Note also that I think (not sure) that "Signed-off-by" is no more mandatory, the commit author is enough now.

AFAIK, that's not longer required for the comment at the PR itself.
But for the commits, it still seems to be required. At least PR #130 was not working without.
And eca-rest-api still mentions it.

[boaks]

@sbernard31

We have to switch to Windows and Internet Explorer, only that sees the CAPTCHA :-).

I opened a ticket about that in the helpdesk

(Using Windows and Internet Explorer I was able to successfully revalidate the PR.)

[sbernard31]

We have to switch to Windows and Internet Explorer, only that sees the CAPTCHA :-).

😱

I opened a ticket about that in the helpdesk

Just see via the related issue that leshan web site doesn't work too. 😬

[obgm]

I would consider, that the size must be initialized depending on the "platform".

Yes, size would be platform dependent in the sense that the address format may differ. Whether or not the port is in fact irrelevant as long as it is the same for all platforms and processed accordingly.

[boaks]

Whether or not the port is in fact irrelevant as long as it is the same for all platforms and processed accordingly.

I guess, the term port doesn't refer to the UDP service port, or?

[leandrolanzieri]

I would consider, that the size must be initialized depending on the "platform".

Yes, size would be platform dependent in the sense that the address format may differ. Whether or not the port is in fact irrelevant as long as it is the same for all platforms and processed accordingly.

This is not the case if the size member is not initialized manually and dtls_session_init is used instead (which seems like the correct thing to do).

IMO it makes sense that the UDP port is part of the cookie, otherwise we may have the case of repeated cookies for handshakes initiated from different ports. I think it would make sense to force that addr contains everything that represents in the particular platform a remote endpoint.

[leandrolanzieri]

I would consider, that the size must be initialized depending on the "platform".

Yes, size would be platform dependent in the sense that the address format may differ. Whether or not the port is in fact irrelevant as long as it is the same for all platforms and processed accordingly.

Just as a reference that it would be good to consider the port for the cookie generation: RFC 6347 Section 4.2.1 states that:

When client sends its ClientHello message to the server, the server MAY respond with a HelloVerifyRequest message. This message contains a stateless cookie generated using the technique of PHOTURIS.

Which references to RFC 2522. Under its Section 3.3 it states:

1. The cookie MUST depend on the specific parties. This prevents an attacker from obtaining a cookie using a real IP address and UDP port, and then using it to swamp the victim with requests from randomly chosen IP addresses or ports.

And in Section 3.3.1:

It is recommended that the cookie be calculated over the secret value, the IP Source and Destination addresses, and the UDP Source and Destination ports.

[boaks]

If the term port is used for the "UDP service port", yes. I agree, it should be considered for the cookie and peer's identity as well. leave it out creates especially in test- and demo-setup irritation, because there usually peers colocated on one host.

But I'm not sure, if port means something as the "software port for a specific platform".

[leandrolanzieri]

But I'm not sure, if port means something as the "software port for a specific platform".

In RIOT it has always been initialized to the remote endpoint UDP port.

https://github.com/RIOT-OS/RIOT/blob/fca56ba0c3383f713179546e1a3a9b5bc2f24fd8/pkg/tinydtls/contrib/sock_dtls.c#L818-L820

[boaks]

portmaybe used as term for the result of porting software. It's then not related to IP at all.

[boaks]

But I think, the discussion shows, that the concepts behind the fields size and addr are not understood well enough to "port" them to an other platform. Also, because the documentation is somehow missing.

size reflects the actual size of the used part of addr, depending on the platform and address family as IPv4 and IPv6. If tinydtls is ported to a new platform, the platform must ensure, that the value of size is adapted.
If that's static on a platform, then it's easy. If not, it must be adapted. Reading the original issue in RIOT my impression was, that the large IPv6 address is filled only with the IPv4 data and results in some random bytes. If the size get's adapted to the current address family, the solution should work.

[leandrolanzieri]

But I think, the discussion shows, that the concepts behind the fields size and addr are not understood well enough to "port" them to an other platform. Also, because the documentation is somehow missing.

As port is defined only for RIOT and Contiki, it seems that they were introduced by the platform porting, so the interpretation the platforms make of it should be the correct one. Speaking from RIOT, session_t::port has always been considered the remote endpoint UDP port.

size reflects the actual size of the used part of addr, depending on the platform and address family as IPv4 and IPv6.

I don't agree. Going back to the implementation of dtls_session_init, size can be determined at compile time, and has nothing to do with the amount of actual used bytes in addr.

https://github.com/eclipse/tinydtls/blob/706888256c3e03d9fcf1ec37bb1dd6499213be3c/session.c#L66

If tinydtls is ported to a new platform, the platform must ensure, that the value of size is adapted.

Not according dtls_session_init.

Reading the original issue in RIOT my impression was, that the large IPv6 address is filled only with the IPv4 data and results in some random bytes.

This was not the issue. We were using only IPv6, as IPv4 was not there yet. But as size was manually set, padding bytes were not considered, introducing uninitialized bytes to the cookie generation.

If the size get's adapted to the current address family, the solution should work.

As suggested above, I think the best way is to group all the information about the remote endpoint in addr (IPv6 or IPv4 + UDP port), enforce that dtls_session_init is used (thus ensuring the setting to 0 of all bytes) and having a unified way of setting session_t::size.

[boaks]

As port is defined only for RIOT and Contiki,

port (in the meaning of UDP service port) is also contained in the posix struct sockaddr_in and so indirect in the addr field.

This was not the issue.

OK, thanks for clarification.

group all the information about the remote endpoint in addr (IPv6 or IPv4 + UDP port), enforce that dtls_session_init is used (thus ensuring the setting to 0 of all bytes) and having a unified way of setting session_t::size.

agreed.

[leandrolanzieri]

How should we proceed? I was thinking maybe we can define types per platform port:

#ifdef WITH_CONTIKI
#include "ip/uip.h"

typedef unsigned char addrlen_t;

typedef struct {
  uip_ipaddr_t addr;      /**< session IP address */
  unsigned short port;    /**< transport layer port */
} addr_t;

 /* TODO: Add support for RIOT over sockets  */
#elif defined(WITH_RIOT_GNRC)
#include "net/ipv6/addr.h"
typedef unsigned char addrlen_t;

typedef struct {
  unsigned short port;  /**< transport layer port */
  ipv6_addr_t addr6;    /**< IPv6 address */
} addr_t;

#else /* ! WITH_CONTIKI && ! WITH_RIOT_GNRC */

#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>

typedef socklen_t addrlen_t;

typedef union {
  struct sockaddr sa;
  struct sockaddr_storage st;
  struct sockaddr_in sin;
  struct sockaddr_in6 sin6;
} addr_t;

#endif /* ! WITH_CONTIKI && ! WITH_RIOT_GNRC */

/**
 * @brief Session identification descriptor.
 */
typedef struct {
  addrlen_t size;   /**< size of session_t::addr */
  addr_t addr;      /**< remote endpoint IP address + UDP port */
  int ifindex;      /**< network interface index */
} session_t;

Maybe instead of addr something like remote would fit better. Also, I was thinking maybe we can split the types and also the implementations of _dtls_address_equals_impl into files in platform-specific.

[boaks]

FMPOV:

Tinydtls itself (the library, not the examples) depends not on something as the representation of the IP address.
It depends on a "couple of bytes", defining the other peer.
Tinydtls use the session_t in three functions:
dtls_create_cookie : here the "size/addr"
dtls_session_equals: here the defined and used fields are compared.
dtls_get_peer: here it's used in HASH_FINDwith "sizeof(session_t)/session" (and some more functions for the peer management using the same).

To clean that up:
Unify the data to be considered by the tinydtls library

In my opinion, the tinydtls library itself should just go for the dtls_get_peer approach and then it's not relevant, how such a session_t is structured, it's relevant, that the data filled in is standardized for each supported OS. So I would also adapt dtls_create_cookie and dtls_session_equals to use to complete session_t. Or did I oversee something?

For the usage on the application layer (e.g. "dtls_client.c"), it's nice to use the data required to easily interact with the API, so the same would apply for RIOT. And the main function required is just the dtls_session_init initializing the session_t to 0s. It then up to the application / port to offer a set of functions to fill the data.

That's very close to the current implementation, but without accessing the "addr" (or any other field).

@leandrolanzieri

But as size was manually set, padding bytes were not considered, introducing uninitialized bytes to the cookie generation.

I may be wrong, but that sounds, that dtls_session_init wasn't called, or?

(If I'm wrong, the the approach to "always use the complete session_t" would not work.)

[leandrolanzieri]

I may be wrong, but that sounds, that dtls_session_init wasn't called, or?

It was not called, because the size it would set does not consider the port, so it was set manually. The issue was that session_t was in consequence not initialized to 0. But this is now fixed by RIOT-OS/RIOT#17849

[obgm]

Thanks, @boaks and @leandrolanzieri for the thorough analysis. I like @boaks's suggested abstraction for cookie creation, session comparison and peer retrieval. And, as these functions need to include some means of identifying the remote peer, the low-level part needs to be platform-specific as suggested by @leandrolanzieri.

I am happy to process this PR (especially considering that PR #129 depends on this and is already in use for RIOT) and do the suggested improvements later. What do you think?

[boaks]

Yes, merging this now and address the general remarks later makes sense.

[obgm]

And here we go!