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.

Sign up for GitHub

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

app_rpt.c: Fix truncated link list in RPT_LINK #470

Closed
wants to merge 1 commit into from
+32 −11
Closed

app_rpt.c: Fix truncated link list in RPT_LINK #470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions apps/app_rpt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1660,16 +1660,24 @@ static inline void init_text_frame(struct ast_frame *wf)
}

static void handle_link_data(struct rpt *myrpt, struct rpt_link *mylink, char *str)
{
char tmp[512], tmp1[512], cmd[300] = "", dest[300], src[30], c;
{
/* These temporary var sizes are a bit tricky:
* tmp is a copy of a string that can be MAXLINKLIST (or larger)
* when we recieve an 'L' it copies tmp into l->linklist which is
* MAXLINKLIST long.
*/
char tmp[MAXLINKLIST], tmp1[512], cmd[300] = "", dest[300], src[30], c;
int i, seq, res, ts, rest;
struct ast_frame wf;

init_text_frame(&wf);
wf.datalen = strlen(str) + 1;
wf.src = "handle_link_data";
/* put string in our buffer */
ast_copy_string(tmp, str, sizeof(tmp) - 1);
if (strlen(str) > sizeof(tmp)) {
ast_log(LOG_WARNING, "Link text message data has exceeded tmp buffer size");
}
ast_copy_string(tmp, str, sizeof(tmp));

ast_debug(5, "Received text over link: '%s'\n", str);

Expand Down Expand Up @@ -1703,15 +1711,15 @@ static void handle_link_data(struct rpt *myrpt, struct rpt_link *mylink, char *s
}
if (tmp[0] == 'L') {
rpt_mutex_lock(&myrpt->lock);
strcpy(mylink->linklist, tmp + 2);
strcpy(mylink->linklist, tmp + 2); /* MAXLINKLIST - 2 is placed in mylink->linklist*/
time(&mylink->linklistreceived);
rpt_mutex_unlock(&myrpt->lock);
ast_debug(7, "@@@@ node %s recieved node list %s from node %s\n", myrpt->name, tmp, mylink->name);
return;
}
if (tmp[0] == 'M') {
rest = 0;
if (sscanf(tmp, "%s %s %s %n", cmd, src, dest, &rest) < 3) {
if (sscanf(tmp, "%s %s %s %n", cmd, src, dest, &rest) < 3) { /*TODO: We should limit to sizeof(cmd, src, dest)*/
Copy link
Member

Choose a reason for hiding this comment

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

It seems like tmp is mutable since we already have a copy of it (I haven't fully verified though)
Instead of the existing code, maybe we should replace with strsep for the first 3 strings and just use sscanf for the numbers? That way we can fix the issue more robustly as opposed to making a TODO?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't disagree we should fix this, but in the interest of keeping this PR pointed at the actual issue maybe do it in a new PR? I'm thinking there would be a "general" fix up sscanf buffer over potentials everywhere, and hopefully the same way.
As for scanf, would %299s protect us with a lighter touch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe something like this....

#define SIZE = 300
char format[20];
char one[SIZE], two[SIZE], three[SIZE]
sprintf(format,"%%%ds %%%ds %%%ds",SIZE-1,SIZE-1,SIZE-1);
sscanf(s, format, one, two, three);

ast_log(LOG_WARNING, "Unable to parse message string %s\n", str);
return;
}
Expand Down Expand Up @@ -1739,7 +1747,7 @@ static void handle_link_data(struct rpt *myrpt, struct rpt_link *mylink, char *s
return;
}
if (tmp[0] == 'T') {
if (sscanf(tmp, "%s %s %s", cmd, src, dest) != 3) {
if (sscanf(tmp, "%s %s %s", cmd, src, dest) != 3) { /*TODO: We should limit to sizeof(cmd, src, dest)*/
ast_log(LOG_WARNING, "Unable to parse telem string %s\n", str);
return;
}
Expand Down Expand Up @@ -1771,7 +1779,7 @@ static void handle_link_data(struct rpt *myrpt, struct rpt_link *mylink, char *s
}

if (tmp[0] == 'C') {
if (sscanf(tmp, "%s %s %s %s", cmd, src, tmp1, dest) != 4) {
if (sscanf(tmp, "%s %s %s %s", cmd, src, tmp1, dest) != 4) { /*TODO: We should limit to sizeof(cmd, src, dest, tmp1)*/
ast_log(LOG_WARNING, "Unable to parse ctcss string %s\n", str);
return;
}
Expand All @@ -1790,7 +1798,7 @@ static void handle_link_data(struct rpt *myrpt, struct rpt_link *mylink, char *s
}

if (tmp[0] == 'K') {
if (sscanf(tmp, "%s %s %s %d %d", cmd, dest, src, &seq, &ts) != 5) {
if (sscanf(tmp, "%s %s %s %d %d", cmd, dest, src, &seq, &ts) != 5) { /*TODO: We should limit to sizeof(cmd, src, dest)*/
ast_log(LOG_WARNING, "Unable to parse keying string %s\n", str);
return;
}
Expand Down Expand Up @@ -2139,10 +2147,16 @@ static int handle_remote_dtmf_digit(struct rpt *myrpt, char c, char *keyed, int

static int handle_remote_data(struct rpt *myrpt, char *str)
{
char tmp[300], cmd[300], dest[300], src[300], c;
/* These temporary var sizes are a bit tricky:
* tmp is a copy of a string that can be MAXLINKLIST (or larger)
*/
char tmp[MAXLINKLIST], cmd[300], dest[300], src[300], c;
int seq, res;

/* put string in our buffer */
if (strlen(str) > sizeof(tmp)) {
ast_log(LOG_WARNING, "Remove link text message data has exceeded tmp buffer size");
}
ast_copy_string(tmp, str, sizeof(tmp));
if (!strcmp(tmp, DISCSTR))
return 0;
Expand All @@ -2167,7 +2181,7 @@ static int handle_remote_data(struct rpt *myrpt, char *str)

#ifndef DO_NOT_NOTIFY_MDC1200_ON_REMOTE_BASES
if (tmp[0] == 'I') {
if (sscanf(tmp, "%s %s %s", cmd, src, dest) != 3) {
if (sscanf(tmp, "%s %s %s", cmd, src, dest) != 3) { /*TODO: these should be limited to sizeof(cmd,dest,src)*/
ast_log(LOG_WARNING, "Unable to parse ident string %s\n", str);
return 0;
}
Expand All @@ -2177,7 +2191,7 @@ static int handle_remote_data(struct rpt *myrpt, char *str)
#endif
if (tmp[0] == 'L')
return 0;
if (sscanf(tmp, "%s %s %s %d %c", cmd, dest, src, &seq, &c) != 5) {
if (sscanf(tmp, "%s %s %s %d %c", cmd, dest, src, &seq, &c) != 5) { /*TODO: these should be limited to sizeof(cmd,dest,src)*/
ast_log(LOG_WARNING, "Unable to parse link string %s\n", str);
return 0;
}
Expand Down
7 changes: 7 additions & 0 deletions apps/app_rpt/rpt_link.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,13 @@ void __mklinklist(struct rpt *myrpt, struct rpt_link *mylink, char *buf, int fla
if (flag) {
snprintf(buf + spos, MAXLINKLIST - spos, "%s%c%c", l->name, mode, (l->lastrx1) ? 'K' : 'U');
} else {

/* l->linklist and buff have a size of MAXLINKLIST. We will have a problem with large networks where this exceeds MAXLINKLIST
* worst case(ish) The message looks like this "<network size>, 6 digit node + T + ,"
* With MAXLINKLIST = 5120, 5120 - 4 / 8 = 638 nodes. We can have smaller node values but generally we
* will have problems on networks > 638 nodes as is.
*/

/* add nodes into buffer */
if (l->linklist[0]) {
snprintf(buf + spos, MAXLINKLIST - spos, "%c%s,%s", mode, l->name, l->linklist);
Expand Down
Loading