From: Pablo Neira Ayuso &lt;pablo(a)gnumonks.org&gt; Now ipaccess_idtag_parse() returns -EINVAL instead of -1. We also check for the return value of this function in every invocation to skip further processing in case of messages with malformed TLVs. This idea was suggested by Zecke. --- openbsc/src/libabis/input/ipaccess.c | 14 +++++++++----- openbsc/src/osmo-bsc_nat/bsc_nat.c | 8 +++++++- openbsc/src/osmo-bsc_nat/bsc_ussd.c | 8 +++++++- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/openbsc/src/libabis/input/ipaccess.c b/openbsc/src/libabis/input/ipaccess.c index c4c13d7..ba641a8 100644 --- a/openbsc/src/libabis/input/ipaccess.c +++ b/openbsc/src/libabis/input/ipaccess.c @@ -123,7 +123,7 @@ int ipaccess_idtag_parse(struct tlv_parsed *dec, unsigned char *buf, int len) if (t_len > len + 1) { LOGP(DMI, LOGL_ERROR, "The tag does not fit: %d\n", t_len); - return -1; + return -EINVAL; } DEBUGPC(DMI, "%s='%s' ", ipaccess_idtag_name(t_tag), cur); @@ -251,7 +251,7 @@ static int ipaccess_rcvmsg(struct e1inp_line *line, struct msgb *msg, u_int16_t site_id = 0, bts_id = 0, trx_id = 0; struct gsm_bts *bts; char *unitid; - int len; + int len, ret; /* handle base messages */ ipaccess_rcvmsg_base(msg, bfd); @@ -260,10 +260,14 @@ static int ipaccess_rcvmsg(struct e1inp_line *line, struct msgb *msg, case IPAC_MSGT_ID_RESP: DEBUGP(DMI, "ID_RESP "); /* parse tags, search for Unit ID */ - ipaccess_idtag_parse(&tlvp, (u_int8_t *)msg->l2h + 2, - msgb_l2len(msg)-2); + ret = ipaccess_idtag_parse(&tlvp, (u_int8_t *)msg->l2h + 2, + msgb_l2len(msg)-2); DEBUGP(DMI, "\n"); - + if (ret < 0) { + LOGP(DINP, LOGL_ERROR, "ignoring IPA response message " + "with malformed TLVs\n"); + return ret; + } if (!TLVP_PRESENT(&tlvp, IPAC_IDTAG_UNIT)) break; diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat.c b/openbsc/src/osmo-bsc_nat/bsc_nat.c index 7586294..8c164a2 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_nat.c +++ b/openbsc/src/osmo-bsc_nat/bsc_nat.c @@ -1019,9 +1019,15 @@ exit: /* do we know who is handling this? */ if (msg->l2h[0] == IPAC_MSGT_ID_RESP) { struct tlv_parsed tvp; - ipaccess_idtag_parse(&tvp, + int ret; + ret = ipaccess_idtag_parse(&tvp, (unsigned char *) msg->l2h + 2, msgb_l2len(msg) - 2); + if (ret < 0) { + LOGP(DNAT, LOGL_ERROR, "ignoring IPA response " + "message with malformed TLVs\n"); + return ret; + } if (TLVP_PRESENT(&tvp, IPAC_IDTAG_UNITNAME)) ipaccess_auth_bsc(&tvp, bsc); } diff --git a/openbsc/src/osmo-bsc_nat/bsc_ussd.c b/openbsc/src/osmo-bsc_nat/bsc_ussd.c index ff1d27a..4beef7b 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_ussd.c +++ b/openbsc/src/osmo-bsc_nat/bsc_ussd.c @@ -123,9 +123,15 @@ static int ussd_read_cb(struct bsc_fd *bfd) if (hh->proto == IPAC_PROTO_IPACCESS) { if (msg->l2h[0] == IPAC_MSGT_ID_RESP) { struct tlv_parsed tvp; - ipaccess_idtag_parse(&tvp, + int ret; + ret = ipaccess_idtag_parse(&tvp, (unsigned char *) msg->l2h + 2, msgb_l2len(msg) - 2); + if (ret < 0) { + LOGP(DNAT, LOGL_ERROR, "ignoring IPA response " + "message with malformed TLVs\n"); + return ret; + } if (TLVP_PRESENT(&tvp, IPAC_IDTAG_UNITNAME)) ussd_auth_con(&tvp, conn); } -- 1.7.2.3

From: Pablo Neira Ayuso &lt;pablo(a)gnumonks.org&gt; This patch was suggested by Zecke. --- openbsc/src/libabis/input/ipaccess.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/openbsc/src/libabis/input/ipaccess.c b/openbsc/src/libabis/input/ipaccess.c index ba641a8..cba03a0 100644 --- a/openbsc/src/libabis/input/ipaccess.c +++ b/openbsc/src/libabis/input/ipaccess.c @@ -200,7 +200,7 @@ static int ipaccess_send(int fd, const void *msg, size_t msglen) if (ret < 0) return ret; if (ret < msglen) { - DEBUGP(DINP, "ipaccess_send: short write\n"); + LOGP(DINP, LOGL_ERROR, "ipaccess_send: short write\n"); return -EIO; } return ret; -- 1.7.2.3

From: Pablo Neira Ayuso &lt;pablo(a)gnumonks.org&gt; With this patch, ipaccess-proxy makes more robust option checking: $ ./ipaccess-proxy -l 1.1.1.1 -b 2.2.2.2 -e ERROR: missing mandatory argument for `-e' option And we return to shell to enforce the user to try again with the appropriate invocation. Before this patch, the default getopt_long() error handling was enabled which displayed this message: ./ipaccess-proxy: option requires an argument -- 'e' and ipaccess-proxy continued working. This is generic enough to cover other option that require mandatory arguments like `--bsc' and `--listen'. --- openbsc/src/ipaccess/ipaccess-proxy.c | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-) diff --git a/openbsc/src/ipaccess/ipaccess-proxy.c b/openbsc/src/ipaccess/ipaccess-proxy.c index 55658cf..dd0a739 100644 --- a/openbsc/src/ipaccess/ipaccess-proxy.c +++ b/openbsc/src/ipaccess/ipaccess-proxy.c @@ -1133,6 +1133,9 @@ static void handle_options(int argc, char** argv) { int options_mask = 0; + /* disable explicit missing arguments error output from getopt_long */ + opterr = 0; + while (1) { int option_index = 0, c; static struct option long_options[] = { @@ -1176,6 +1179,18 @@ static void handle_options(int argc, char** argv) case 'e': log_set_log_level(stderr_target, atoi(optarg)); break; + case '?': + if (optopt) { + printf("ERROR: missing mandatory argument " + "for `%s' option\n", argv[optind-1]); + } else { + printf("ERROR: unknown option `%s'\n", + argv[optind-1]); + } + print_usage(); + print_help(); + exit(EXIT_FAILURE); + break; default: /* ignore */ break; -- 1.7.2.3

From: Pablo Neira Ayuso &lt;pablo(a)gnumonks.org&gt; It is similar to make_sock() available in openbsc/libcommon. --- openbsc/src/ipaccess/ipaccess-proxy.c | 40 +++++--------------------------- 1 files changed, 7 insertions(+), 33 deletions(-) diff --git a/openbsc/src/ipaccess/ipaccess-proxy.c b/openbsc/src/ipaccess/ipaccess-proxy.c index 4dd6ee5..5254959 100644 --- a/openbsc/src/ipaccess/ipaccess-proxy.c +++ b/openbsc/src/ipaccess/ipaccess-proxy.c @@ -126,7 +126,6 @@ static char *listen_ipaddr; static char *bsc_ipaddr; static char *gprs_ns_ipaddr; -static int make_gprs_sock(struct bsc_fd *bfd, int (*cb)(struct bsc_fd*,unsigned int), void *); static int gprs_ns_cb(struct bsc_fd *bfd, unsigned int what); #define PROXY_ALLOC_SIZE 1200 @@ -389,7 +388,13 @@ static int ipbc_alloc_connect(struct ipa_proxy_conn *ipc, struct bsc_fd *bfd, if (gprs_ns_ipaddr) { struct sockaddr_in sock; socklen_t len = sizeof(sock); - ret = make_gprs_sock(&ipbc->gprs_ns_fd, gprs_ns_cb, ipbc); + struct in_addr addr; + uint32_t ip; + + inet_aton(listen_ipaddr, &addr); + ip = ntohl(addr.s_addr); /* make_sock() needs host byte order */ + ret = make_sock(&ipbc->gprs_ns_fd, IPPROTO_UDP, ip, 0, 0, + gprs_ns_cb, ipbc); if (ret < 0) { LOGP(DINP, LOGL_ERROR, "Creating the GPRS socket failed.\n"); goto err_udp_bsc; @@ -982,37 +987,6 @@ static int gprs_ns_cb(struct bsc_fd *bfd, unsigned int what) return 0; } -static int make_gprs_sock(struct bsc_fd *bfd, int (*cb)(struct bsc_fd*,unsigned int), void *data) -{ - struct sockaddr_in addr; - int ret; - - bfd->fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP); - bfd->cb = cb; - bfd->data = data; - bfd->when = BSC_FD_READ; - - memset(&addr, 0, sizeof(addr)); - addr.sin_family = AF_INET; - addr.sin_port = 0; - inet_aton(listen_ipaddr, &addr.sin_addr); - - ret = bind(bfd->fd, (struct sockaddr *) &addr, sizeof(addr)); - if (ret < 0) { - LOGP(DINP, LOGL_ERROR, - "Could not bind n socket for IP %s with error: %s.\n", - listen_ipaddr, strerror(errno)); - return -EIO; - } - - ret = bsc_register_fd(bfd); - if (ret < 0) { - perror("register_listen_fd"); - return ret; - } - return 0; -} - /* Actively connect to a BSC. */ static struct ipa_proxy_conn *connect_bsc(struct sockaddr_in *sa, int priv_nr, void *data) { -- 1.7.2.3