search

dbus2 / zbus

Rust D-Bus crate.
Other
370 stars 84 forks source link

NUL (`'\0'`) bytes within Rust strings cause issues #128

Closed zeenix closed 1 year ago

zeenix commented 3 years ago

In GitLab by @tmuehlbacher on Jan 22, 2021, 19:19

It's unlikely that someone actually wants to transmit strings that contain NUL in them. However in some applications it may be possible to get such strings into a D-Bus interface and cause it to hang.

Using this small example with zbus = 1.8.0 and zvariant = 2.4.0:

use std::convert::TryInto;
use std::error::Error;

use zbus::Connection;
use zbus::ObjectServer;
use zbus::dbus_interface;
use zbus::fdo;

struct Smth {
    whatever: String,
}

#[dbus_interface(name = "org.example.Smth1")]
impl Smth {
    #[dbus_interface(property)]
    fn whatever(&self) -> &str {
        &self.whatever
    }
}

fn main() -> Result<(), Box<dyn Error>> {
    let conn = Connection::new_session()?;

    fdo::DBusProxy::new(&conn)?.request_name(
        "org.example.Smth1",
        fdo::RequestNameFlags::ReplaceExisting.into(),
    )?;

    let mut objserv = ObjectServer::new(&conn);

    let smth = Smth { whatever: String::from("hey, that's a string with 3 NULs\0\0\0") };

    objserv.at(&"/org/example/Smth".try_into()?, smth)?;
    loop {
        if let Err(err) = objserv.try_handle_next() {
            eprintln!("{}", err);
        }
    }
}

As a surface level way of debugging this, let's run it with strace

$ strace ./target/debug/smth

Then do something that tries to read the property:

$ busctl --user introspect org.example.Smth1 /org/example/Smth

This is the last usable part of the trace, recvmsg is just endlessly called until the program is killed.

getuid()                                = 1000
sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\0AUTH EXTERNAL 31303030\r\n", iov_len=25}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 25
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="OK c025737ba4fae3b1cfd49233c8110"..., iov_len=40}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 37
sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="NEGOTIATE_UNIX_FD\r\n", iov_len=19}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 19
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="AGREE_UNIX_FD\r\n", iov_len=40}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 15
sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="BEGIN\r\n", iov_len=7}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 7
getrandom("\x51\xe1\x0e\xb1\xf5\xa5\xa8\x5f\x7f\xbf\x05\x9e\xf9\xf2\x8a\x19", 16, GRND_NONBLOCK) = 16
sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="l\1\0\1\0\0\0\0\1\0\0\0m\0\0\0\1\1o\0\25\0\0\0/org/fre"..., iov_len=128}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 128
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="l\2\1\1\v\0\0\0\377\377\377\377?\0\0\0", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 16
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="\5\1u\0\1\0\0\0\7\1s\0\24\0\0\0org.freedesktop."..., iov_len=75}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 75
sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="l\1\0\1\34\0\0\0\2\0\0\0\215\0\0\0\10\1g\0\2su\0\7\1s\0\6\0\0\0"..., iov_len=188}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 188
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="l\4\1\1\v\0\0\0\377\377\377\377\217\0\0\0", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 16
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="\7\1s\0\24\0\0\0org.freedesktop.DBus\0\0\0\0"..., iov_len=155}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 155
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="l\4\1\1\26\0\0\0\377\377\377\377\217\0\0\0", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 16
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="\7\1s\0\24\0\0\0org.freedesktop.DBus\0\0\0\0"..., iov_len=166}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 166
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="l\2\1\1\4\0\0\0\377\377\377\377?\0\0\0", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 16
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="\5\1u\0\2\0\0\0\7\1s\0\24\0\0\0org.freedesktop."..., iov_len=68}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 68
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="l\1\0\1\0\0\0\0\2\0\0\0\227\0\0\0", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 16
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="\1\1o\0\21\0\0\0/org/example/Smth\0\0\0\0\0\0\0"..., iov_len=152}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 152
sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="l\2\0\1\220\5\0\0\3\0\0\0/\0\0\0\10\1g\0\1s\0\0\7\1s\0\6\0\0\0"..., iov_len=1488}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 1488
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="l\1\0\1\26\0\0\0\3\0\0\0\217\0\0\0", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 16
recvmsg(3, {msg_name={sa_family=AF_UNIX, sun_path="/run/user/1000/bus"}, msg_namelen=128->21, msg_iov=[{iov_base="\1\1o\0\21\0\0\0/org/example/Smth\0\0\0\0\0\0\0"..., iov_len=166}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 166
sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="l\2\0\1?\0\0\0\4\0\0\0007\0\0\0\10\1g\0\5a{sv}\0\0\0\0\0\0"..., iov_len=135}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 135
recvmsg(3, {msg_name=0x7ffd87585cc8, msg_namelen=128->0, msg_iov=[{iov_base="", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 0
recvmsg(3, {msg_name=0x7ffd87585cc8, msg_namelen=128->0, msg_iov=[{iov_base="", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 0
recvmsg(3, {msg_name=0x7ffd87585cc8, msg_namelen=128->0, msg_iov=[{iov_base="", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 0
recvmsg(3, {msg_name=0x7ffd87585cc8, msg_namelen=128->0, msg_iov=[{iov_base="", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 0
recvmsg(3, {msg_name=0x7ffd87585cc8, msg_namelen=128->0, msg_iov=[{iov_base="", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 0
recvmsg(3, {msg_name=0x7ffd87585cc8, msg_namelen=128->0, msg_iov=[{iov_base="", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 0
recvmsg(3, {msg_name=0x7ffd87585cc8, msg_namelen=128->0, msg_iov=[{iov_base="", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 0
recvmsg(3, {msg_name=0x7ffd87585cc8, msg_namelen=128->0, msg_iov=[{iov_base="", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 0
recvmsg(3, {msg_name=0x7ffd87585cc8, msg_namelen=128->0, msg_iov=[{iov_base="", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 0
recvmsg(3, {msg_name=0x7ffd87585cc8, msg_namelen=128->0, msg_iov=[{iov_base="", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 0
recvmsg(3, {msg_name=0x7ffd87585cc8, msg_namelen=128->0, msg_iov=[{iov_base="", iov_len=16}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 0
...

I think that since Rust String/str with interior NULs in them are completely valid, this should be handled in some way by zvariant, I suppose?

zeenix commented 3 years ago

@sdroege any advice?

zeenix commented 3 years ago

In GitLab by @sdroege on Jan 23, 2021, 11:14

I don't know what exactly happens here, but my guess would be that you use CStr / CString somewhere or do something like strlen()? Don't use any of those, then this should work just fine in theory with strings containing interior \0s.

zeenix commented 3 years ago

I don't know what exactly happens here, but my guess would be that you use CStr / CString somewhere or do something like strlen()?

Actually no. On the serializer side, we just send the string w/o checking for an embedded NULL and that's the issue here cause that's not allowed in D-Bus:

UTF-8 string (must be valid UTF-8). Must be nul terminated and contain no other nul bytes.

Maybe the solution is simply to check for those and error out on it. That'd mean us checking every byte of every string though. :(

zeenix commented 3 years ago

In GitLab by @sdroege on Jan 23, 2021, 16:00

Yes that would seem like the only option then.

zeenix commented 3 years ago

In GitLab by @tmuehlbacher on Jan 23, 2021, 17:13

So this currently passes just fine:

    let ctxt = Context::<LE>::new_dbus(0);

    let encoded = to_bytes(ctxt, &"hello\0world").unwrap();
    let decoded: &str = from_slice(&encoded, ctxt).unwrap();
    assert_eq!(decoded, "hello\0world");

One solution would be to have the above example fail at unwrapping to_bytes().

Or another solution would be to just truncate the strings, for example:

    let ctxt = Context::<LE>::new_dbus(0);

    let encoded = to_bytes(ctxt, &"hello\0world").unwrap();
    let decoded: &str = from_slice(&encoded, ctxt).unwrap();
    assert_eq!(decoded, "hello");

If this was chosen it would definitely have to be documented very well.

zeenix commented 3 years ago

One solution would be to have the above example fail at unwrapping to_bytes().

Yeah, that's what I meant above.

Or another solution would be to just truncate the strings, for example:

I thought of that too but I wasn't sure if it's right to do that silently.

If this was chosen it would definitely have to be documented very well.

I know Rust allows null bytes in strings but is that really so commonly used? Not saying we shouldn't document though.

zeenix commented 3 years ago

In GitLab by @tmuehlbacher on Jan 23, 2021, 19:18

I know Rust allows null bytes in strings but is that really so commonly used? Not saying we shouldn't document though.

When I noticed this issue in my case it was definitely erroneous to have NUL in the string. I honestly don't know if interior NULs have any valid or common use case in Rust atm, but I also don't have tons of experience in Rust yet.

Another issue is the deser part. If I understand it correctly, serialized strings have a length before the actual data and then also a trailing NUL? So it would be easily possible to actually construct something invalid by inserting a NUL at any offset <= length? In this case, I think the more obvious solution would be to throw an error when decoding, as it is definitely invalid input according to the D-Bus spec, right?\ My current guess is that this is probably something the dbus-broker/dbus-daemon checks for and won't transmit anything that's invalid? But that's something I would really have to check and it's probably smart to be resilient and not just trust in that. Also it doesn't explain the issue that Bilal ran into with deserializing.

The more I think about it, I would actually prefer it to throw an error when serializing. It would be in line with deserializing (potentially) and be less likely to cause any confusion (maybe someone expects the string to not get truncated at NUL but for the NULs to be stripped, which may require an allocation). Basically for API users say: "When serializing, you (zbus user) need to make sure your strings follow the simple rules in the D-Bus spec of being valid UTF-8 (automatic in Rust strings) and not containing NUL."

zeenix commented 3 years ago

Another issue is the deser part. If I understand it correctly, serialized strings have a length before the actual data and then also a trailing NUL? So it would be easily possible to actually construct something invalid by inserting a NUL at any offset <= length? In this case, I think the more obvious solution would be to throw an error when decoding, as it is definitely invalid input according to the D-Bus spec, right?

Correct.

My current guess is that this is probably something the dbus-broker/dbus-daemon checks for and won't transmit anything that's invalid?

Yeah.

Also it doesn't explain the issue that Bilal ran into with deserializing.

True but I'm not sure what their issue really is as i never got all the details. It's possible that their problem is with gvariant format, not dbus.

The more I think about it, I would actually prefer it to throw an error when serializing. It would be in line with deserializing (potentially) and be less likely to cause any confusion (maybe someone expects the string to not get truncated at NUL but for the NULs to be stripped, which may require an allocation).

:thumbsup: The code is here, if you want to contribute this.

Basically for API users say: "When serializing, you (zbus user) need to make sure your strings follow the simple rules in the D-Bus spec of being valid UTF-8 (automatic in Rust strings) and not containing NUL."

Yeah but UTF-8 part need not be explicitly mentioned as that's always the case.

zeenix commented 3 years ago

mentioned in commit 489f2af780c0a3d61eca1cc5c1ebc9225fa3a226