Proxmark3 community

Andreas

Member

Registered: 2010-01-25

Posts: 10

DbpString has a high chance of overwriting stack space

Sorry for reposting this topic but first time it ended up in the wrong section.

I have noticed that DbpString is using a buffer of 48 bytes.

There is a high chance of exceeding this size on mistake while writing a debug statement or an error message.

The longest strings I found so far in the source code is 47 bytes:
(not shore if it isn't pure luck that this limit is not exceeded)

appmain.cpp: DbpString("Measuring antenna characteristics, please wait.");

I suggest to add a sanity check to reduce the impact of to long strings.

Index: armsrc/appmain.c
===================================================================
--- armsrc/appmain.c    (revision 317)
+++ armsrc/appmain.c    (working copy)
@@ -75,10 +75,14 @@

        UsbCommand c;
        c.cmd = CMD_DEBUG_PRINT_STRING;
        c.arg[0] = strlen(str);
+       if(c.arg[0] > sizeof(c.d.asBytes))
+       {
+               c.arg[0] = sizeof(c.d.asBytes);
+       }
        memcpy(c.d.asBytes, str, c.arg[0]);
       UsbSendPacket((BYTE *)&c, sizeof(c));
        // TODO fix USB so stupid things like this aren't req'd
        SpinDelay(50);
 }

atrox

Contributor

Registered: 2010-01-08

Posts: 35

Re: DbpString has a high chance of overwriting stack space

why is there a limitation of 48 bytes ? (downloading samples is also done in 48 bytes pieces) is this a usb limitation ?

Andreas

Member

Registered: 2010-01-25

Posts: 10

Re: DbpString has a high chance of overwriting stack space

The total payload size sent per packet is 64byte.

:
typedef struct {
    uint32_t    cmd;
    uint32_t    arg[3];
    union {
        uint8_t     asBytes[48];
        uint32_t    asDwords[12];
    } d;
} PACKED UsbCommand;

If I remember the USB specification correctly:
high speed devices are allowed to use a packet size of 8,16, 32 and 64byte
full speed devices have to use 64 byte.

Last edited by Andreas (2010-05-01 11:37:26)