Bug #3420
openranap_transp_assoc_decode() decodes X.213 NSAP address headers as IP address
30%
Description
The SysmoCell5000 series sends RAB-AssignmentResponse with X.213 NSAP addresses, which get parsed by ranap_transp_assoc_decode().
These contain a three byte header followed by the IP address, typically it is 0x350001 followed by the Ipv4 address in hex.
ranap_transp_assoc_decode() however starts at the header and decodes as 53.0.1.N where N is the first octet of the actual IPv4 address, e.g. 192.
Hence switching RTP streams to the cell fails, since obviously 53.0.1.192 is not the correct IP address.
a) fix the check for the address length, should be len == 7, not len > 7.
b) generally be stricter about the lengths.
Checklist
- fix the check for the address length, should be len == 7, not len > 7
- generally be stricter about the lengths, e.g. do not decode a len > 4 as IPv4
- properly decode the X.213 NSAP header, knowing what the octets mean instead of using magic numbers and fixed data offsets
- implement a C unit test
Updated by neels almost 6 years ago
osmo-iuh iu_helpers.c
/* decode a network address as used inside ASN.1 encoded Iu interface protocols */ int ranap_transp_layer_addr_decode(char *addr, unsigned int addr_len, const RANAP_TransportLayerAddress_t *trasp_layer_addr) { unsigned char *buf; int len; const char *rc; buf = trasp_layer_addr->buf; len = trasp_layer_addr->size; if (buf[0] == 0x35 && len > 7) rc = inet_ntop(AF_INET, buf + 3, addr, addr_len); else if (len > 3) rc = inet_ntop(AF_INET, buf, addr, addr_len); else return -EINVAL; if (!rc) return -EINVAL; return 0; }
This is so wrong on various levels.
Updated by neels almost 6 years ago
- Checklist item fix the check for the address length, should be len == 7, not len > 7 set to Done
the current failure is mended by trivial https://gerrit.osmocom.org/#/c/osmo-iuh/+/10545 ,
other decoding improvements remain pending.
Updated by neels almost 6 years ago
- Status changed from New to Stalled
- % Done changed from 0 to 30
Updated by neels almost 6 years ago
That fix which changed the condition to '== 7' actually broke nano3G voice calls.
To hackily accomodate both sysmocell and nano3G, it must be '>= 7' instead :/
https://gerrit.osmocom.org/#/c/osmo-iuh/+/11167
Updated by osmith over 5 years ago
neels wrote in the commit message of the last patch:
A proper X.213 NSAP decoding would still be more welcome than this patching back and forth, but this is (another) quick fix without spending too much time on it.
So I looked into what a proper X.213 NSAP decoding would be. The reference for having IPv4 encoded in NSAPA is defined in RFC4538 ยง 3.1.
It matches exactly what you have found, the 0x350001 followed by the IPv4 is the standard. The header for IPv6 is 0x350000.
=> We can update the code to check for 0x350001 now, not only for 0x35.
The length is always 20 bytes in the document. However, as the address is stored in Osmocom as transportLayerAddress, it gets cut off and that sort of implies what kind of address it has been. I did not find yet where it gets saved and cut up initially (will look into that tomorrow, that is probably the key to why we have different lengths for NSAP addresses that should have the same length), but I did find another piece of code that extracts the IPv4 from the transportLayerAddress in osmo-sgsn/src/gprs/sgsn_libgtp.c:sgsn_ranap_rab_ass_resp():
switch (item->transportLayerAddress->size) { case 7: /* It must be IPv4 inside a X213 NSAP */ memcpy(pdp->lib->gsnlu.v, &item->transportLayerAddress->buf[3], 4); break; case 4: /* It must be a raw IPv4 address */ memcpy(pdp->lib->gsnlu.v, item->transportLayerAddress->buf, 4); break; case 16: /* TODO: It must be a raw IPv6 address */ case 19: /* TODO: It must be IPv6 inside a X213 NSAP */ default: LOGP(DRANAP, LOGL_ERROR, "RAB Assignment Resp: Unknown " "transport layer address size %u\n", item->transportLayerAddress->size); return -1; }
Updated by osmith over 5 years ago
Summary¶
IPv4 Addresses get written into RANAP_TransportLayerAddress_t in various formats. The
ranap_transp_layer_addr_decode() and sgsn_ranap_rab_ass_resp() functions interpret it as follows:
Source | length | header bytes | bytes containing the IPv4 | |
---|---|---|---|---|
1. | (unknown) | 4 (or more) | none | 0-3 |
2. | nano3g | > 7 (probably 20) | 0x350001 | 4-7 |
3. | sysmocell | 7 | 0x350001 | 4-7 |
So this is a mixture of writing the real IPv4 address (1.) (possibly with unneeded bytes that follow) and full/cut-off versions of IPv4 Address Encoding in an NSAPA (2. and 3.). IPv6 addresses are not interpreted by both functions (there is a standard for embedding them in NSAP addresses).
A lot of the osmo-iuh C code is generated from ASN1 files. The relevant datatype seems to be defined in RANAP-IEs.asn:
TransportLayerAddress ::= BIT STRING (SIZE (1..160, ...))
How to continue?¶
I thought the TransportLayerAddress would get written somewhere in the Osmocom code, and we could simply write the real address without a header and with a fixed length, and it would be fixed. But it seems that the addresses get sent from the nano3g, sysmocell or other devices instead. Does this mean, where the format does not match the spec, that this is a bug in these devices, and we can't really do anything about it instead of guess the address that has been sent like we are doing now?
Maybe we could move that guessing code into a central location instead of having it twice in both functions?
Updated by neels over 5 years ago
We need to be able to parse various formats coming from various 3G cells.
We also need to encode either raw IPv4 or a valid X.213 depending on osmo-msc.cfg.
Parsing in one central location sounds good, I wasn't aware that we had two different places?
(Would be even nicer to autodetect which format we should send, but I think that's not easily possible; during RAB assignment, we first send an address and then get one back, so we can't mimick the cell's format)
Updated by laforge over 3 years ago
osmith ping? would be great to get this wrapped up after two years stalled.
Updated by osmith almost 3 years ago
- Assignee changed from osmith to neels
neels, I looked into this again and I'm not sure if there is anything left to do here. From the issue description on top:
a) fix the check for the address length, should be len == 7, not len > 7.
Turned out it needed to be >= 7, and you've fixed that already in https://gerrit.osmocom.org/c/osmo-iuh/+/11167.
b) generally be stricter about the lengths.
As we need to support the different formats that the devices send, it seems that we can't be more strict about lengths? I guess we could possibly write the detection more nicely, but I'm not sure if that is worth it or how it should be done.
Can you please close the issue, or write down what needs to be done exactly?
Updated by neels almost 3 years ago
I'm quite far removed from this issue by now, and haven't been involved in any 3G for a while now.
It seems that the "proper" thing would be to write up a proper decoder for X.213,
write a C unit test for it to ensure that it decodes both the 7 byte as well as the padded 20 bytes long versions.
Then looking at this code:
if (buf[0] == 0x35 && len >= 7) rc = inet_ntop(AF_INET, buf + 3, addr, addr_len); else if (len > 3) rc = inet_ntop(AF_INET, buf, addr, addr_len); else return -EINVAL;
something like this would look more proper, pseudocode:
if (decode_x213(&x213, buf, len) == 0) { switch(x213.af) { case AF_INET: inet_ntop(x213.af, x213.in_addr, addr, addr_len); break; case AF_INET6: inet_ntop(x213.af, x213.in6_addr, addr, addr_len); break; default: return -EINVAL; } } else if (len == 4) { inet_ntop(AF_INET, buf, addr, addr_len); }
something like that anyway...
Not sure if we really care that much at the moment?