Project

General

Profile

Actions

Bug #3420

open

ranap_transp_assoc_decode() decodes X.213 NSAP address headers as IP address

Added by neels over 5 years ago. Updated over 2 years ago.

Status:
Stalled
Priority:
Normal
Assignee:
Target version:
-
Start date:
07/26/2018
Due date:
% Done:

30%

Spec Reference:

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
Actions #1

Updated by neels over 5 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.

Actions #2

Updated by neels over 5 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.

Actions #3

Updated by neels over 5 years ago

  • Status changed from New to Stalled
  • % Done changed from 0 to 30
Actions #4

Updated by laforge over 5 years ago

  • Assignee changed from neels to osmith
Actions #5

Updated by neels over 5 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

Actions #6

Updated by osmith over 5 years ago

  • Status changed from Stalled to In Progress
Actions #7

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;
        }
Actions #8

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?

Actions #9

Updated by osmith over 5 years ago

  • Status changed from In Progress to Stalled
Actions #10

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)

Actions #11

Updated by laforge about 3 years ago

osmith ping? would be great to get this wrapped up after two years stalled.

Actions #12

Updated by osmith over 2 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?

Actions #13

Updated by neels over 2 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?

Actions #14

Updated by neels over 2 years ago

  • Assignee changed from neels to osmith
Actions

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)