Hello,
I have found a couple bugs in the read callback of smpp_smsc.c with regards to guarding against malformed packets.
Please see attached patches for fixes. They are also published in openbsc branch daniel/smpp-fixes
Regarding the last fix I don't think it is necessary to receive messages up to SSIZE_MAX, but since I don't have/know a value for the maximum sensible size I left it like that for now.
Regards, Daniel
Read returning -1 is an error here so make sure and print the actual reason and close the socket. Otherwise we just loop over the fd with read returning -1 every time.
EINTR is handled to not cause an error and because the socket is not opened with O_NONBLOCK we don't need to check EAGAIN/EWOULDBLOCK. --- openbsc/src/libmsc/smpp_smsc.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/openbsc/src/libmsc/smpp_smsc.c b/openbsc/src/libmsc/smpp_smsc.c index 64ed200..943464f 100644 --- a/openbsc/src/libmsc/smpp_smsc.c +++ b/openbsc/src/libmsc/smpp_smsc.c @@ -778,8 +778,12 @@ static int esme_link_read_cb(struct osmo_fd *ofd) rdlen = sizeof(uint32_t) - esme->read_idx; rc = read(ofd->fd, lenptr + esme->read_idx, rdlen); if (rc < 0) { - LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d\n", - esme->system_id, rc); + /* EINTR is a non-fatal error, just try again */ + if (errno == EINTR) + return 0; + LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d (%s)\n", + esme->system_id, rc, strerror(errno)); + goto dead_socket; } else if (rc == 0) { goto dead_socket; } else @@ -801,8 +805,9 @@ static int esme_link_read_cb(struct osmo_fd *ofd) rdlen = esme->read_len - esme->read_idx; rc = read(ofd->fd, msg->tail, OSMO_MIN(rdlen, msgb_tailroom(msg))); if (rc < 0) { - LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d\n", - esme->system_id, rc); + LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d (%s)\n", + esme->system_id, rc, strerror(errno)); + goto dead_socket; } else if (rc == 0) { goto dead_socket; } else {
On Fri, Jan 17, 2014 at 05:26:26PM +0100, Daniel Willmann wrote:
LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d\n",esme->system_id, rc);
/* EINTR is a non-fatal error, just try again */if (errno == EINTR)return 0;
if (rc < 0) {
LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d\n",esme->system_id, rc);
LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d (%s)\n",esme->system_id, rc, strerror(errno));goto dead_socket;
For reference. you should make the errno check in both cases or just simplify the reading part and move the error handling to a macro.
On Fri, 2014-01-17 at 18:23, Holger Hans Peter Freyther wrote:
On Fri, Jan 17, 2014 at 05:26:26PM +0100, Daniel Willmann wrote:
For reference. you should make the errno check in both cases or just simplify the reading part and move the error handling to a macro.
Good catch, thanks. I made the checks into a macro which should eventually end up in libosmocore so it's usable by other projects as well.
I found another issue as well which I rolled into my last patch. msgb_alloc takes a uint16_t as size so the check was still wrong.
I rebased and pushed the new patches to daniel/smpp-fixes
Regards, Daniel
On Mon, Jan 20, 2014 at 07:47:23PM +0100, Daniel Willmann wrote:
I found another issue as well which I rolled into my last patch. msgb_alloc takes a uint16_t as size so the check was still wrong.
good catch! Shall we export something from MSGB to indicate the maximum msgb size?
On Mon, 2014-01-20 at 20:28, Holger Hans Peter Freyther wrote:
On Mon, Jan 20, 2014 at 07:47:23PM +0100, Daniel Willmann wrote:
I found another issue as well which I rolled into my last patch. msgb_alloc takes a uint16_t as size so the check was still wrong.
good catch! Shall we export something from MSGB to indicate the maximum msgb size?
I'm not opposed, but I don't see how it would help anything. I doubt that whoever forgot to check the function signature will remember to compare the length against a define.
Regards, Daniel
On Tue, Jan 21, 2014 at 02:32:57PM +0100, Daniel Willmann wrote:
I'm not opposed, but I don't see how it would help anything. I doubt that whoever forgot to check the function signature will remember to compare the length against a define.
Maybe it is time for gerrit or at least a set-up for patchwork. I had a look at your branch and stopped at the first commit message. :)
"Before this patch we just loop over the fd with read returning -1.."
==> looped (past tense)?
"EINTR is handled to not cause an error and because the socket is not opened with O_NONBLOCK we don't need to check EAGAIN/EWOULDBLOCK."
I don't think it is true. osmo_fd_register will enable O_NONBLOCK on the socket and I think we use that to register the fd.
the actual fixes look nice though. :)
On Sun, 2014-01-26 at 08:44, Holger Hans Peter Freyther wrote:
Maybe it is time for gerrit or at least a set-up for patchwork. I had a look at your branch and stopped at the first commit message. :)
In an attempt to prevent issues with my other patches I reviewed them again and found some more issues. I pushed an updated and rebased version to daniel/smpp-fixes and will send the patchset here as well.
"Before this patch we just loop over the fd with read returning -1.." ==> looped (past tense)?
Right, fixed.
I don't think it is true. osmo_fd_register will enable O_NONBLOCK on the socket and I think we use that to register the fd.
You're right, the reason is wrong. Checking for these codes is still not needed, though (since we get called only if there's something to read.
the actual fixes look nice though. :)
At least something :-)
Regards, Daniel
From: Daniel Willmann dwillmann@sysmocom.de
Read returning -1 is an error here so make sure to print the actual reason and close the socket. Before this patch we just looped over the fd with read returning -1 every time.
EINTR is handled to not cause an error and we don't need to check EAGAIN/EWOULDBLOCK since the callback is only called in case there is something to read.
To avoid copy&paste issues the check is implemented as a macro and the log message moved into a separate if. --- openbsc/src/libmsc/smpp_smsc.c | 47 ++++++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 16 deletions(-)
diff --git a/openbsc/src/libmsc/smpp_smsc.c b/openbsc/src/libmsc/smpp_smsc.c index 64ed200..1e9829b 100644 --- a/openbsc/src/libmsc/smpp_smsc.c +++ b/openbsc/src/libmsc/smpp_smsc.c @@ -762,6 +762,23 @@ static int smpp_pdu_rx(struct osmo_esme *esme, struct msgb *msg __uses) return rc; }
+/* This macro should be called after a call to read() in the read_cb of an + * osmo_fd to properly check for errors. + * rc is the return value of read, err_label is the label to jump to in case of + * an error. The code there should handle closing the connection. + * FIXME: This code should go in libosmocore utils.h so it can be used by other + * projects as well. + * */ +#define OSMO_FD_CHECK_READ(rc, err_label) \ + if (rc < 0) { \ + /* EINTR is a non-fatal error, just try again */ \ + if (errno == EINTR) \ + return 0; \ + goto err_label; \ + } else if (rc == 0) { \ + goto err_label; \ + } + /* !\brief call-back when per-ESME TCP socket has some data to be read */ static int esme_link_read_cb(struct osmo_fd *ofd) { @@ -777,13 +794,13 @@ static int esme_link_read_cb(struct osmo_fd *ofd) case READ_ST_IN_LEN: rdlen = sizeof(uint32_t) - esme->read_idx; rc = read(ofd->fd, lenptr + esme->read_idx, rdlen); - if (rc < 0) { - LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d\n", - esme->system_id, rc); - } else if (rc == 0) { - goto dead_socket; - } else - esme->read_idx += rc; + if (rc < 0) + LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d (%s)\n", + esme->system_id, rc, strerror(errno)); + OSMO_FD_CHECK_READ(rc, dead_socket); + + esme->read_idx += rc; + if (esme->read_idx >= sizeof(uint32_t)) { esme->read_len = ntohl(len); msg = msgb_alloc(esme->read_len, "SMPP Rx"); @@ -800,15 +817,13 @@ static int esme_link_read_cb(struct osmo_fd *ofd) msg = esme->read_msg; rdlen = esme->read_len - esme->read_idx; rc = read(ofd->fd, msg->tail, OSMO_MIN(rdlen, msgb_tailroom(msg))); - if (rc < 0) { - LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d\n", - esme->system_id, rc); - } else if (rc == 0) { - goto dead_socket; - } else { - esme->read_idx += rc; - msgb_put(msg, rc); - } + if (rc < 0) + LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d (%s)\n", + esme->system_id, rc, strerror(errno)); + OSMO_FD_CHECK_READ(rc, dead_socket); + + esme->read_idx += rc; + msgb_put(msg, rc);
if (esme->read_idx >= esme->read_len) { rc = smpp_pdu_rx(esme, esme->read_msg);
From: Daniel Willmann dwillmann@sysmocom.de
The first 4 bytes are the length including the length field. For length < 4 the subsequent msgb_put(msg, sizeof(uint32_t)) will fail, resulting in an abort. The code also expects (in smpp_msgb_cmdid()) the existence of 4 more bytes for the SMPP command ID.
This patch checks that the length received is large enough to hold all 8 bytes in the msgb and drops the connection if that's not the case.
The issue is reproducible with: echo -e "\x00\x00\x00\x02\x00" |socat stdin tcp:localhost:2775 --- openbsc/src/libmsc/smpp_smsc.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/openbsc/src/libmsc/smpp_smsc.c b/openbsc/src/libmsc/smpp_smsc.c index 1e9829b..605bdd5 100644 --- a/openbsc/src/libmsc/smpp_smsc.c +++ b/openbsc/src/libmsc/smpp_smsc.c @@ -803,6 +803,12 @@ static int esme_link_read_cb(struct osmo_fd *ofd)
if (esme->read_idx >= sizeof(uint32_t)) { esme->read_len = ntohl(len); + if (esme->read_len < 8) { + LOGP(DSMPP, LOGL_ERROR, "[%s] read length too small %u\n", + esme->system_id, esme->read_len); + goto dead_socket; + } + msg = msgb_alloc(esme->read_len, "SMPP Rx"); if (!msg) return -ENOMEM;
From: Daniel Willmann dwillmann@sysmocom.de
The size parameter of msgb_alloc is uint16_t so any length value above 65535 will allocate a msgb with incorrect size.
This patch changes the type of rdlen and rc to ssize_t (the return value of read) and guards against the read length being larger than UINT16_MAX.
To reproduce the issue run: echo -en "\x00\x01\x00\x01\x01" |socat stdin tcp:localhost:2775 --- openbsc/src/libmsc/smpp_smsc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/openbsc/src/libmsc/smpp_smsc.c b/openbsc/src/libmsc/smpp_smsc.c index 605bdd5..048c1b8 100644 --- a/openbsc/src/libmsc/smpp_smsc.c +++ b/openbsc/src/libmsc/smpp_smsc.c @@ -23,6 +23,7 @@ #include <string.h> #include <stdint.h> #include <errno.h> +#include <limits.h>
#include <sys/socket.h> #include <netinet/in.h> @@ -787,15 +788,14 @@ static int esme_link_read_cb(struct osmo_fd *ofd) uint8_t *lenptr = (uint8_t *) &len; uint8_t *cur; struct msgb *msg; - int rdlen; - int rc; + ssize_t rdlen, rc;
switch (esme->read_state) { case READ_ST_IN_LEN: rdlen = sizeof(uint32_t) - esme->read_idx; rc = read(ofd->fd, lenptr + esme->read_idx, rdlen); if (rc < 0) - LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d (%s)\n", + LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %zd (%s)\n", esme->system_id, rc, strerror(errno)); OSMO_FD_CHECK_READ(rc, dead_socket);
@@ -803,8 +803,8 @@ static int esme_link_read_cb(struct osmo_fd *ofd)
if (esme->read_idx >= sizeof(uint32_t)) { esme->read_len = ntohl(len); - if (esme->read_len < 8) { - LOGP(DSMPP, LOGL_ERROR, "[%s] read length too small %u\n", + if (esme->read_len < 8 || esme->read_len > UINT16_MAX) { + LOGP(DSMPP, LOGL_ERROR, "[%s] length invalid %u\n", esme->system_id, esme->read_len); goto dead_socket; } @@ -824,7 +824,7 @@ static int esme_link_read_cb(struct osmo_fd *ofd) rdlen = esme->read_len - esme->read_idx; rc = read(ofd->fd, msg->tail, OSMO_MIN(rdlen, msgb_tailroom(msg))); if (rc < 0) - LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d (%s)\n", + LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %zd (%s)\n", esme->system_id, rc, strerror(errno)); OSMO_FD_CHECK_READ(rc, dead_socket);
The first four bytes are the length including the length field. For length < 4 the subsequent msgb_put(msg, sizeof(uint32_t)) will fail, resulting in an abort.
This patch guards against this problem by closing the connection if the length received is < 5 (since no payload does not make any sense).
The issue is reproducible with: echo -e "\x00\x00\x00\x02\x00" |socat stdin tcp:localhost:2775 --- openbsc/src/libmsc/smpp_smsc.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/openbsc/src/libmsc/smpp_smsc.c b/openbsc/src/libmsc/smpp_smsc.c index 943464f..a3dc311 100644 --- a/openbsc/src/libmsc/smpp_smsc.c +++ b/openbsc/src/libmsc/smpp_smsc.c @@ -790,6 +790,12 @@ static int esme_link_read_cb(struct osmo_fd *ofd) esme->read_idx += rc; if (esme->read_idx >= sizeof(uint32_t)) { esme->read_len = ntohl(len); + if (esme->read_len <= 4) { + LOGP(DSMPP, LOGL_ERROR, "[%s] read length too small %d\n", + esme->system_id, esme->read_len); + goto dead_socket; + } + msg = msgb_alloc(esme->read_len, "SMPP Rx"); if (!msg) return -ENOMEM;
In case the length is 2^31+4 or larger we compute a negative value to read in rdlen (since it is a signed int). This results in read failing with EFAULT.
This patch changes the type of rdlen and rc to ssize_t (the return value of read) and guards against the read length being larger than (SSIZE_MAX).
To reproduce the issue run: echo -e "\x80\x00\x00\x05\x00" |socat stdin tcp:localhost:2775 --- openbsc/src/libmsc/smpp_smsc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/openbsc/src/libmsc/smpp_smsc.c b/openbsc/src/libmsc/smpp_smsc.c index a3dc311..23acfc2 100644 --- a/openbsc/src/libmsc/smpp_smsc.c +++ b/openbsc/src/libmsc/smpp_smsc.c @@ -23,6 +23,7 @@ #include <string.h> #include <stdint.h> #include <errno.h> +#include <limits.h>
#include <sys/socket.h> #include <netinet/in.h> @@ -770,8 +771,7 @@ static int esme_link_read_cb(struct osmo_fd *ofd) uint8_t *lenptr = (uint8_t *) &len; uint8_t *cur; struct msgb *msg; - int rdlen; - int rc; + ssize_t rdlen, rc;
switch (esme->read_state) { case READ_ST_IN_LEN: @@ -781,7 +781,7 @@ static int esme_link_read_cb(struct osmo_fd *ofd) /* EINTR is a non-fatal error, just try again */ if (errno == EINTR) return 0; - LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d (%s)\n", + LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %zd (%s)\n", esme->system_id, rc, strerror(errno)); goto dead_socket; } else if (rc == 0) { @@ -790,8 +790,8 @@ static int esme_link_read_cb(struct osmo_fd *ofd) esme->read_idx += rc; if (esme->read_idx >= sizeof(uint32_t)) { esme->read_len = ntohl(len); - if (esme->read_len <= 4) { - LOGP(DSMPP, LOGL_ERROR, "[%s] read length too small %d\n", + if (esme->read_len <= 4 || esme->read_len > SSIZE_MAX) { + LOGP(DSMPP, LOGL_ERROR, "[%s] length invalid %d\n", esme->system_id, esme->read_len); goto dead_socket; } @@ -811,7 +811,7 @@ static int esme_link_read_cb(struct osmo_fd *ofd) rdlen = esme->read_len - esme->read_idx; rc = read(ofd->fd, msg->tail, OSMO_MIN(rdlen, msgb_tailroom(msg))); if (rc < 0) { - LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %d (%s)\n", + LOGP(DSMPP, LOGL_ERROR, "[%s] read returned %zd (%s)\n", esme->system_id, rc, strerror(errno)); goto dead_socket; } else if (rc == 0) {
-
Daniel Willmann -
Daniel Willmann -
Holger Hans Peter Freyther