MetaDataValidation

[tpaetz]

It is possible to crash receivers by sending messages with corrupted meta data information, because there is no validation of the meta data length that is send. Thus, other OpenIGTLink end points can easily be crashed by sending such messages.

Furthermore, such a failed message unpacking is not propagated along the call hierarchy resulting in further processing of corrupted/invalid messages.

• ensure that amount of bytes read from metaData does not exceed expected meta data length
• do not set m_IsBodyUnpacked and UNPACK_BODY if unpacking failed

[leochan2009]

Hi Torben,

Could you please add some test to this new feature? The test should cover the handling of crash scenario. Basically the unpackMetaData() should return false instead of crash. The test can be added to the igtlImageBaseTest. Thanks

[leochan2009]

Hi Torben, In the example you assumed that during the data transmission, the bytes can be corrupted/modified. In this scenario, what can be done is actually use the crccheck. if you set the crccheck to be true in the unpack process, the corrupted data can be detected. There is no need to check the meta information. in this line TestCode you can write this : EXPECT_NE(statusReceiveMsg->Unpack(1), igtl::MessageBase::UNPACK_BODY);

[tpaetz]

I know that my example is superficial.

In our use case we communicate with another OpenIGTLink implementation that may construct messages with invalid meta data size information. Thus, CRC are correct because the data was already wrong before checksum calculation.

Reproducing this in a test would require to write an own Message class. Thus, I just artificially corrupted the meta data size after packing to ease the test setup.

[leochan2009]

Hi Torben,

now i understand the issue, however, i try to understand why "another OpenIGTLink implementation that may construct messages with invalid meta data size information". If you look into the meta data example, https://github.com/openigtlink/OpenIGTLink/blob/master/Testing/igtlImageMetaMessageTest.cxx the meta data size is calculated by the library. How this other opengitlink implementation contruct the message? can you show me some sample code?

[tpaetz]

The message generation in the other implementation can be fixed, this is not my point here. The underlying issue I see is that an attacker can easily crash all programs that use OpenIGTLink communication based on the "official" OpenIGTLink implementation. This requires to send a message that contains the demonstrated content only (of course with a matching CRC after the modification, but this is on the sender side as well).

[leochan2009]

if we are talking about man-in-the-middle attack. I think this pull request will only fix the meta data modification attack and won't fix the man-in-the-middle issue in general. The security is never considered in Openigtlink, only integrity is checked. zeromq might be a better solution if security is considered.

[leochan2009]

My points are:

1. The issue was a result of inappropriate use of the library
2. The general man-in-the-middle attack won't be fixed by adding numerous checks int the code. The encryption and authentication will be needed to fix the attack issue. unfortunately encryption and authentication are never considered in this library.
3. Adding a lot of check will make the currently already unreadable code more complex.

[tokjun]

Hi @leochan2009, I agree that the library has a proper mechanism to address a man-in-the-middle attack but can we at least avoid a crash due to a corrupt message? It's a stability issue rather than a security.

[leochan2009]

@tokjun , Yes, this pull request is mainly for handling inappropriately use of the library when adding meta data. it is not for the detection of corrupted message as the crc is correct. it will provide limited help regarding the man in the middle attack. however, it does improve the stability. I will merge the pull in that sense.

[tokjun]

Thanks, @leochan2009! Sorry you probably figured out but in my previous comment, I wanted to say the library does not have a proper mechanism...