dotnet / iot

This repo includes .NET Core implementations for various IoT boards, chips, displays and PCBs.
MIT License
2.12k stars 574 forks source link

TryReadNdefMessage can trim off terminator TLV (0xFE) causing validation to throw exception #2286

Closed pete-intranel closed 3 months ago

pete-intranel commented 4 months ago

Describe the bug

Reading NDEF message who's length is in a 16 byte boundary crashes in NdefMessage.ExtractMessage with IndexOutOfRangeException

This piece of code in UltralightCard.TryReadNdefMessage(...) will trim off the terminator TLV (0xFE) for a payload who's size is a factor of BlockSize

int blocksToRead = (size + start) / BlockSize + ((size + start) % BlockSize != 0 ? 1 : 0);

MifareCard.TryReadNdefMessage(...) appears to do something similar but slightly differently, but I haven't tried it.

Then this code (in NdefMessage.ExtractMessage) throws the exception:

bool isRealEnd = toExtract[idx + size] == 0xFE;

I find it hard to believe I'm the only one to have seen this, so perhaps the issue lies somewhere else?

If this is a real bug, I'm not sure on the best approach to fix it.

Option 1: ExtractMessage could loosen its constraints (the specs seem to indicate that the terminator is optional anyway?), e.g.

// Finally check that the optional end terminator TLV is 0xFE
bool isRealEnd = (toExtract.Length == idx + size) || (toExtract[idx + size] == 0xFE);

Option 2: Include the terminator by reading an extra block:

int blocksToRead = (size + start) / BlockSize + 1;

Option 3: Both option 1 and 2

Ellerbach commented 4 months ago

@pete-intranel thanks a lot for the investigation and reporting the issue.

I researched a bit more, and right, the structure does not always need to finish by 0xFE. It means that your option 1 would be the best.

If you are ready for a PR, please go ahead and I'll be happy to review it!