NVM Group 1 data CRC never validated on LmHandler NVM Restore

[dmeehan1968]

The following code is never executed due to the conditon always being false.

The effect is that the Group 1 data is not checked for validity before restoration, which could lead to a corruption.

Why does this condition exist?

https://github.com/Lora-net/LoRaMac-node/blob/55f83f90ddbc4bc6aabd34a3262d3870548b3577/src/apps/LoRaMac/common/NvmDataMgmt.c#L189-L195

[janakj]

Looks correct to me. It only checks the CRC value if there actually is any data (other than the CRC) in RegionNvmDataGroup1_t. The size of that data structure depends on the included regions. If you end up with a RegionNvmDataGroup1_t structure that only contains the CRC value, e.g., if your only supported region is EU868, there is no need to check the CRC.

[dmeehan1968]

Ah yes, I misread it.

Would it not be better to put this in the CRC check function, to return true if the size is equal to sizeof uint32?

I was responding to my IDE warning that it was unreachable and assuming the structure would have some data.

[janakj]

This test is a corner case that only needs to be performed for one particular data structure. If you put it in the function, it will be performed every single time the function is invoked, even when not needed. You would get no real gain at the expense of slightly worse performance. Since there appears to be only one place where the test is needed, I am in favor of performing it there. If this becomes a recurring problem at some point, you can always reconsider.

Also, CRC32 returns a well-defined value when computed over an empty block. The value is derived from the initial value of the accumulator. It's zero if I remember correctly, but I haven't checked and I am not quite sure. If the application code depends on this behavior for some reason, doing the check in the function might break it. I couldn't come up with any reasonable use case for computing CRC32 over an empty block, but who knows what's out there.

[dmeehan1968]

Within NvmmCrc32Check, testing whether the input size is greater than sizeof(uint32_t) is not a considerable overhead, and as implemented in NvmmCrc32Check it ignores the possibility of the input size and offset resulting in a negative. In my mind the problem lies in recognising that any of the structures being tested could be zero based on conditional compilation. The same could be true of any of the structures, or any data that NvmmCrc32Check could be applied too in the future. IMHO, its a bug waiting to happen. That NvmmCrc32Check does not validate its input arguments is a potential source of a security vulnerability.

NvmmCrc32Check cannot function with a size less than sizeof(uint32_t), yet blindly performs a calculation that, with unsigned wrap, will result in a runtime assertion (or ignored) rather than being gracefully handled.

It also assumes that none of the other structures, with future amendments, will not result in a zero length structure, without the developer recognising the risk. I argue the overhead of moving the check into the function far outweighs the potential risk.

I do not think that comparing the Crc to a known value is a sensible strategy, as its also a possible legitimate result.

[janakj]

No other NVM data structure can have zero payload size. See their definitions. RegionNvmDataGroup1_t is an exception.

I agree regarding parameter checking. NvmmCrc32Check should check that the size parameter value is at least four bytes. This kind if parameter checking is generally good.

I am not in favor of returning true unconditionally for size == 4, i.e., if it gets a data structure with empty payload. In this case, the function should still check the CRC32 checksum value and return true if it is 0 and false otherwise. All CRC32 libraries that I have worked with support empty payloads. The CRC32 checksum of empty payload is 0.

If you still plan to modify the behavior of the function in the size == 4 case, please consider changing the function's name to something like NvmmCrc32CheckMaybe. That might help alert the programmer to the fact that the function does not perform pure CRC32 verification and they should think twice before they adapt it for use in some other context.

[dmeehan1968]

I shouldn't (nor should any developer) have to check whether any of those structures happens to be zero length to know if the function works as advertised.

@mluis1 Pushed that change only a few days ago so I'd be happy to see what their view is. Also agree that if the CRC as persisted must be zero for a size == 4 call, then that should also be part of the validation.

I think I have a query about whether there is a risk that 4 bytes of uninitialised NVM could return a false positive or negative. Right now it skips over 4 bytes if the struct is only 4 bytes (so silently ignores the content of the NVM at that location).

[janakj]

I guess you could also argue that if RegionNvmDataGroup1_t is empty, its NVM checksum should still have been initialized to 0. Then the if condition in your original post would not be needed. That is, instead of fixing the reader, you would fix the writer.

[dmeehan1968]

Agreed, that's what the original issue that led to the patch outlined. It also proposed what I am proposing - that the check be within NvmmCrc32Check(), not outside it. That if the structure is only a CRC (by size) that it returns true without further validation. @mluis1, for reasons unclear, placed the check at the call site.

The NvmDataMgmt modules are not public facing it seems, so I'm less concerned as I'll likely implement my own management, but the Nvmm module is public facing, and I think should handle the edge case directly. Maybe that assessment is wrong for reasons I'm not aware of. I don't think its worth endless debate, and at least this issue/thread explains the pitfalls to anyone looking.