Skip to content

Conversation

adam-3bian
Copy link

No description provided.

else if (state->bufferIndex < BufferSize - 1)
{
state->buffer[state->bufferIndex++] = byte;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not so familiar with AT command processing but is it standard to just ignore characters in a command after 32 bytes?

Copy link
Collaborator

@rmn30 rmn30 Feb 10, 2025

Choose a reason for hiding this comment

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

I think there's a second (intentional) overflow here if there is a BufferSize length or greater command followed by \n or \r?

Copy link
Author

Choose a reason for hiding this comment

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

For the purposes of a demo, I've limited to 32-bytes. It allowed me to send a sensible input of 33 or above to see if at_state_process_input would stop the overrun.

For example, if you send:

AT=ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789\r\n

The current code should read both \r and \n as new line characters. This isn't standard behaviour. Probably only \r should be used and \n should be ignored if found afterwards.

if (state->buffer[i] == '=')
{
state->buffer[i] = '\0';
argument = &state->buffer[i + 1];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the at_command_set handler assumes this is null terminated so I think I could trigger a crash if it isn't? Potentially a nice demo to have a compartment_error_handler that handles this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also argument is out of bounds if there is a BufferSize command that ends in =.

ATCommand commands[MaxCommands];
uint8_t commandCount;
char buffer[BufferSize];
char argument[ArgumentSize];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This field is unused.

@rmn30
Copy link
Collaborator

rmn30 commented Feb 10, 2025

In case you are wondering why you're not getting traps in your example it may be because we have not turned on sub-object bounds by default so all the overflows of ATState.buffer are actually in the same object. You might want to add -Xclang -cheri-bounds=subobject-safe when compiling. Note that the overflows would still be difficult to exploit except as DoS due to not being able to forge pointers on CHERI.

More info: https://www.cl.cam.ac.uk/~pffm2/sosp2023_cheri_tutorial/exercises/4_subobject-bounds/README.html

@rmn30
Copy link
Collaborator

rmn30 commented Feb 10, 2025

Also re. code style since this is C++ all the functions that take an ATState * should be methods (or constructors) of ATState!

@adam-3bian
Copy link
Author

Some code I've previously kept as C compatible to allow for easy binding to other languages. This was probably OK for a proof of concept, but not for the final solution.

@adam-3bian
Copy link
Author

I need to run another pass over this, hopefully the code takes on board your C/C++ comments. Uses only carriage return now. I still need to implement compartment_error_handler. Ideally the AT commands should be case insensitive.

priority = 1,
entry_point = "entry_point",
stack_size = 0x800,
stack_size = 0x1200,
Copy link
Author

Choose a reason for hiding this comment

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

Stack sizes to be lowered in next commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants