17 Jan
2014
17 Jan
'14
5:26 p.m.
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
--
- Daniel Willmann <dwillmann(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Schivelbeiner Str. 5
* 10439 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
17 Jan
17 Jan
5:26 p.m.
New subject: [PATCH 1/3] smpp_smsc: Print errno and close socket if read returns -1
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 {
--
1.8.4.2
6:23 p.m.
New subject: [PATCH 1/3] smpp_smsc: Print errno and close socket if read returns -1
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.
20 Jan
20 Jan
7:47 p.m.
New subject: [PATCH 1/3] smpp_smsc: Print errno and close socket if read returns -1
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
--
- Daniel Willmann <dwillmann(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Schivelbeiner Str. 5
* 10439 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
8:28 p.m.
New subject: [PATCH 1/3] smpp_smsc: Print errno and close socket if read returns -1
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?
21 Jan
21 Jan
2:32 p.m.
New subject: [PATCH 1/3] smpp_smsc: Print errno and close socket if read returns -1
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'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
--
- Daniel Willmann <dwillmann(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Schivelbeiner Str. 5
* 10439 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
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?
26 Jan
26 Jan
8:44 a.m.
New subject: [PATCH 1/3] smpp_smsc: Print errno and close socket if read returns -1
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. :)
28 Jan
28 Jan
6:26 p.m.
New subject: [PATCH 1/3] smpp_smsc: Print errno and close socket if read returns -1
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
--
- Daniel Willmann <dwillmann(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Schivelbeiner Str. 5
* 10439 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
6:30 p.m.
New subject: [PATCH 1/3] smpp_smsc: Fix socket read() error handling
From: Daniel Willmann <dwillmann(a)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);
--
1.8.4.2
6:30 p.m.
New subject: [PATCH 2/3] smpp_smsc: Check that the size is large enough to hold actual data
From: Daniel Willmann <dwillmann(a)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;
--
1.8.4.2
6:30 p.m.
New subject: [PATCH 3/3] smpp_smsc: Fix integer overflow in read return value and msgb_alloc()
From: Daniel Willmann <dwillmann(a)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);
--
1.8.4.2
17 Jan
17 Jan
5:26 p.m.
New subject: [PATCH 2/3] smpp_smsc: Check that the size is large enough to hold actual data
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;
--
1.8.4.2
5:26 p.m.
New subject: [PATCH 3/3] smpp_smsc: Fix 32-bit signed integer overflow in read return value
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) {
--
1.8.4.2
4162
days inactive
4173
days old
12 comments
3 participants
participants (3)
-
Daniel Willmann -
Daniel Willmann -
Holger Hans Peter Freyther