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

scan_mac_cmds doesn't handle erroneous MAC commands #485

Closed
terrillmoore opened this issue Oct 22, 2019 · 1 comment
Closed

scan_mac_cmds doesn't handle erroneous MAC commands #485

terrillmoore opened this issue Oct 22, 2019 · 1 comment
Assignees
Labels

Comments

@terrillmoore
Copy link
Member

terrillmoore commented Oct 22, 2019

I'm wondering why 0 is returned in the error case, because in scan_mac_cmds I see no check except for line 887, which doesn't do what the comment says.

int const cmdlen = getMacCmdSize(cmd);
if (cmdlen > olen - oidx) {

Originally posted by @sualko in #481

@terrillmoore
Copy link
Member Author

Yes, that's a bug too -- no test cases for bad mac commands, even in certification. (Which is probably a security issue -- we need to be able to fuzz the inputs from the radio.) getMacCmdSize() returns 0 if the command is not a recognized command. Since commands are always at least 1 byte long, this is an unambiguous indication of error; which the caller currently ignores.

The int cmdlen is sort of wrong, too, but chosen because olen and oidx are both int as well. It will be promoted in the comparison anyway, Since getMacCmdSize returns u1_t, we can just compare to zero, because cmdlen can't be negative. (We could also trust the compiler to optimize, but it might be as likely to warn pedantically "cmdlen can never be negative"; or we could change cmdlen to u1_t and let the compiler promote to int during the comparison.)

if (cmdlen == 0 || cmdlen > olen - oidx)

sualko added a commit to sualko/arduino-lmic that referenced this issue Oct 22, 2019
@terrillmoore terrillmoore self-assigned this Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant