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

apply correct bounds for various array inputs #142

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

nathaniel-bennett
Copy link
Contributor

@nathaniel-bennett nathaniel-bennett commented Oct 17, 2024

Fixes several out-of-bounds memory accesses that can be triggered via an unauthenticated cellular device.

Partially deals with #143, though the ASN.1 files for S1AP will still need to be regenerated using a new version of the asn1c compiler (e.g. https://github.com/mouse07410/asn1c) in order to resolve the buffer overflow issues contained within it. It appears to be a static-compiled library that is stored in the git repo and linked at build time.

@omecproject
Copy link

Can one of the admins verify this patch?

3 similar comments
@omecproject
Copy link

Can one of the admins verify this patch?

@omecproject
Copy link

Can one of the admins verify this patch?

@omecproject
Copy link

Can one of the admins verify this patch?

@@ -80,6 +80,11 @@ int convertToInitUeProtoIe(InitiatingMessage_t *msg, struct proto_IE* proto_ies,
return -1;
}

if (s1apNASPDU_p->size > 300) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you arrive at 300?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The destination of the memcpy is s1Msg->nasMsg.nasMsgBuf. s1Msg is of type initial_ue_msg_t, which has the following definition:

struct initial_ue_msg {
s1_incoming_msg_header_t header;
rawNas_Q_msg_t nasMsg;
int enb_fd;
int criticality;
unsigned char IMSI[BINARY_IMSI_LEN];
struct TAI tai;
struct CGI utran_cgi;
enum ie_RRC_est_cause rrc_cause;
struct STMSI s_tmsi;
}__attribute__ ((packed));
typedef struct initial_ue_msg initial_ue_msg_t;

Following from that, the rawNas_Q_msg_t type has definition:

struct rawNas_Q_msg {
uint8_t nasMsgBuf[MAX_NAS_MSG_SIZE];
uint16_t nasMsgSize;
}__attribute__ ((packed));
typedef struct rawNas_Q_msg rawNas_Q_msg_t;

... which actually uses MAX_NAS_MSG_SIZE, equal to 500 bytes. My bad. I think I had seen the general nasMsgBuf field in several other types having a constant of 300, such as:

struct authreq_info {
msg_type_t msg_type;
int ue_idx;
int enb_s1ap_ue_id;
int enb_fd;
uint8_t nasMsgBuf[300];
uint8_t nasMsgSize;
}__attribute__ ((packed));
typedef struct authreq_info authreq_info_t;

... and wrongly assumed they would be consistent across all message types. I'll get that fixed.

@@ -468,6 +473,11 @@ int convertUplinkNasToProtoIe(InitiatingMessage_t *msg, struct proto_IE* proto_i
return -1;
}

if (s1apNASPDU_p->size > 300) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment - how you arrived at 300?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I relied on the same assumption as above, which may be wrong. In this case, s1Msg is of type uplink_nas_t, which is defined here:

struct uplink_nas {
s1_incoming_msg_header_t header;
rawNas_Q_msg_t nasMsg;
int enb_fd;
uint32_t s1ap_enb_ue_id;
uint32_t s1ap_mme_ue_id;
struct TAI tai;
struct CGI utran_cgi;
struct STMSI s_tmsi;
}__attribute__ ((packed));
typedef struct uplink_nas uplink_nas_t;
struct ue_context_rel_req {
s1_incoming_msg_header_t header;
}__attribute__ ((packed));
typedef struct ue_context_rel_req ue_context_rel_req_t;

It likewise uses a rawNas_Q_msg_t:

struct rawNas_Q_msg {
uint8_t nasMsgBuf[MAX_NAS_MSG_SIZE];
uint16_t nasMsgSize;
}__attribute__ ((packed));
typedef struct rawNas_Q_msg rawNas_Q_msg_t;

So yes, it should be changed to MAX_NAS_MSG_SIZE as well. Ill get this fixed too.

@@ -646,6 +660,11 @@ int convertInitCtxRspToProtoIe(SuccessfulOutcome_t *msg, struct proto_IE* proto_

if(s1apErabSetupItem_p->transportLayerAddress.buf != NULL)
{
if (s1apErabSetupItem_p->transportLayerAddress.size != sizeof(int)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May need formatting of the code..seems space & tab combination. Could you pls fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 👍

@gab-arrobo
Copy link

@nathaniel-bennett, you need to "redo" your commit using the -s flag to pass the DCO check. You see details here: https://wiki.linuxfoundation.org/dco

@nathaniel-bennett
Copy link
Contributor Author

Alright, I've gone ahead and force-pushed the requested changes (including -s addition to the commit).

@gab-arrobo gab-arrobo merged commit 8c04550 into omec-project:master Dec 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants