facontidavide/ros_type_introspection

blind/invalid reads

ibtaylor opened this issue · 7 comments

ibtaylor commented

ReadFromBuffer does not know the size of buffer_ptr

ros_type_introspection/src/deserializer.cpp

Line 53 in 81fd041

array_size = ReadFromBuffer<int32_t>( buffer_ptr );

ros_type_introspection/src/deserializer.cpp

Line 64 in 81fd041

size_t string_size = (size_t) ReadFromBuffer<int32_t>( buffer_ptr );

ros_type_introspection/src/parser.cpp

Lines 116 to 210 in 81fd041

if( _msg_name.compare( "bool" ) == 0 ) {
_id = RosIntrospection::BOOL;
_deserialize_impl = [](uint8_t** buffer) {
return ReadFromBuffer<bool>(buffer);
};
}
else if(_msg_name.compare( "byte" ) == 0 ) {
_id = RosIntrospection::BYTE;
_deserialize_impl = [](uint8_t** buffer) {
return ReadFromBuffer<int8_t>(buffer);
};
}
else if(_msg_name.compare( "char" ) == 0 ) {
_id = RosIntrospection::CHAR;
_deserialize_impl = [](uint8_t** buffer) {
return ReadFromBuffer<char>(buffer);
};
}
else if(_msg_name.compare( "uint8" ) == 0 ) {
_id = RosIntrospection::UINT8;
_deserialize_impl = [](uint8_t** buffer) {
return ReadFromBuffer<uint8_t>(buffer);
};
}
else if(_msg_name.compare( "uint16" ) == 0 ) {
_id = RosIntrospection::UINT16;
_deserialize_impl = [](uint8_t** buffer) {
return ReadFromBuffer<uint16_t>(buffer);
};
}
else if(_msg_name.compare( "uint32" ) == 0 ) {
_id = RosIntrospection::UINT32;
_deserialize_impl = [](uint8_t** buffer) {
return ReadFromBuffer<uint32_t>(buffer);
};
}
else if(_msg_name.compare( "uint64" ) == 0 ) {
_id = RosIntrospection::UINT64;
_deserialize_impl = [](uint8_t** buffer) {
return ReadFromBuffer<uint64_t>(buffer);
};
}
else if(_msg_name.compare( "int8" ) == 0 ) {
_id = RosIntrospection::INT8;
_deserialize_impl = [](uint8_t** buffer) {
return ReadFromBuffer<int8_t>(buffer);
};
}
else if(_msg_name.compare( "int16" ) == 0 ) {
_id = RosIntrospection::INT16;
_deserialize_impl = [](uint8_t** buffer) {
return ReadFromBuffer<int16_t>(buffer);
};
}
else if(_msg_name.compare( "int32" ) == 0 ) {
_id = RosIntrospection::INT32;
_deserialize_impl = [](uint8_t** buffer) {
return ReadFromBuffer<int32_t>(buffer);
};
}
else if(_msg_name.compare( "int64" ) == 0 ) {
_id = RosIntrospection::INT64;
_deserialize_impl = [](uint8_t** buffer) {
return ReadFromBuffer<int64_t>(buffer);
};
}
else if(_msg_name.compare( "float32" ) == 0 ) {
_id = RosIntrospection::FLOAT32;
_deserialize_impl = [](uint8_t** buffer) {
return ReadFromBuffer<float>(buffer);
};
}
else if(_msg_name.compare( "float64" ) == 0 ) {
_id = RosIntrospection::FLOAT64;
_deserialize_impl = [](uint8_t** buffer) {
return ReadFromBuffer<double>(buffer);
};
}
else if(_msg_name.compare( "time" ) == 0 ) {
_id = RosIntrospection::TIME;
_deserialize_impl = [](uint8_t** buffer) {
ros::Time tmp;
tmp.sec = ReadFromBuffer<uint32_t>(buffer);
tmp.nsec = ReadFromBuffer<uint32_t>(buffer);
return tmp;
};
}
else if(_msg_name.compare( "duration" ) == 0 ) {
_id = RosIntrospection::DURATION;
_deserialize_impl = [](uint8_t** buffer) {
ros::Time tmp;
tmp.sec = ReadFromBuffer<int32_t>(buffer);
tmp.nsec = ReadFromBuffer<int32_t>(buffer);
return tmp;
};

and so can have an invalid read here

ros_type_introspection/include/ros_type_introspection/parser.hpp

Line 127 in 4782d46

T destination = (*( reinterpret_cast<T*>( *buffer ) ) );

If the string_size is larger than the remaining contents of buffer_ptr, this should be an error. It is not safe to trust the user supplied data.

ros_type_introspection/src/deserializer.cpp

Lines 64 to 65 in 81fd041

size_t string_size = (size_t) ReadFromBuffer<int32_t>( buffer_ptr );
SString id( (const char*)(*buffer_ptr), string_size );

facontidavide commented

I know that this is quite fragile in THEORY, and I have in mind a way to improve it. But if the message is not corrupted, it should always work.

I am curious: have you find a practical situation in which there is a segmentation fault or memory corruption? Can I reproduce it?

facontidavide commented

Take a look to this branch. It should provide a safer alternative

https://github.com/facontidavide/ros_type_introspection/tree/check_buffer_overrun

ibtaylor commented

Just thought I should point it out. It assumes all the data is well formed. This isn't really acceptable if you want to process data from a potentially malicious source (yeah, I know, but I get bags from other people, so..). To actually trigger this, you would have to probably create a malicious bag outside of the ros tool suite.

That branch looks like an improvement on robustness.

I guess I think it might be bad to switch from a pointer to a vector just because now it forces the user to copy their data into a std::vector (since there is no way to wrap an existing buffer using std::vector afaik). What about having them supply void*/uint8_t* and a size_t/ptrdiff_t for the size in bytes of the memory location?

facontidavide commented

Make sense, even if I believe that std ::vector is still a better design. I might also use a template to allow the user to pass std ::vector, std::array or vector_view

facontidavide commented

I found a solution that will make both of us happy.
1159a1a
Now you can pass a buffer as:

  • raw (uint8_t* / size_t) pair for C style arrays
  • std::vector<uint8_t>
  • std::array<uint8_t>
facontidavide commented

changed my mind. std::deque would fail because memory is NOT contiguous

This is the current PR. I am waiting for your feedback before merging it

#11

facontidavide commented

I am temporary closing this issue, since this and other changes will be introduced soon in version 1.0 (currently Pull Request 11).

Once it is merged, all the API braking changes will be introduced at once. I also wrote a new funtion to Extract only part of a message, for example std_msgd::header ;)

https://github.com/facontidavide/ros_type_introspection/blob/check_buffer_overrun/include/ros_type_introspection/deserializer.hpp#L275