📝 zb: Add security documentation & warnings to system methods

[complexspaces]

This PR closes #1275 by adding basic and sufficient, but attention-grabbing, documentation to all system* methods in the crate which either are or utilize the result of zbus::Address::system to describe their security properties.

I took the approach to centralize the details/important parts on zbus::Address::system and then use interdoc links to redirect all of the other "affected" callsites there to reduce duplication.

Here are some examples of how rustdoc renders it when built locally:

When viewing the documentation through an editor (in my case VSCode), the warning rendering isn't shown (its a rustdoc specific markdown extension) but the Security header is nicely large:

[zeenix]

Nice! Could you please just add the emoji prefix to the commit and PR titles, as per the contribution guidelines? To help you avoid having to copy&paste it from gitmoji.dev, I'll do that for the PR title and you can just c&p it from there on the git message.

[zeenix]

I'll do that for the PR title and you can just c&p it from there on the git message.

Done. Oh and while you're at it, please also add description to the commit message. Just c&p from the PR description.

[complexspaces]

I've updated the commit's information. Sorry for missing that the first time around.

[zeenix]

Thanks for updating! LGTM otherwise.

[zeenix]

you can remove this paragraph if you use my suggestion above.

[zeenix]

sorry but I only noticed now: This method does not return an address. We need to make it more specific to the problematic bit:

Suggested change

	/// `zbus` does not perform any security checks or trust validation on the address (path)
	/// returned by this method.
	/// Note that `zbus` does not (and can not) validate the value of the `DBUS_SYSTEM_BUS_ADDRESS`
	/// environment variable before using it. Users in security-sensitive contexts (ex: authentication)
	/// are responsible for ensuring that the value of this environment variable can be trusted.

[complexspaces]

FWIW I don't agree with the exact wording here. I think its better for users to validate the value the function returns and not its inputs independently.

[zeenix]

Then we disagree on what's expected of the user. 😆 Perhaps I'm missing something but:

• how does the user verify the connection is valid? I'd imagine it'd be easier to ensure that the env is correct/clean. I think this is what was suggested in the summary of the CVE you pointed to:

libdbus maintainers state that this is a vulnerability in the applications that do not cleanse environment variables

• the damage could already be done by the time user has a connection instance, since the connection creation involves a handshake and "Hello" message being sent out to the peer.

[zeenix]

@complexspaces are you convinced by my arguments? If not, could you please reply? I understand if you don't want to argue about this anymore but in that case, kindly accept my proposed wording so we can move on here. 🙏

[complexspaces]

Hey again, sorry, I ended up at a conference that took most of my time and attention.

how does the user verify the connection is valid? I'd imagine it'd be easier to ensure that the env is correct/clean. I think this is what was suggested in...

My argument here is that I'm not usually a fan of recommending users inspect all of the inputs to a function because its more fragile to drift over time or missing a possible branch. Instead, its easier to create an Address::system and analyze that as a "final piece" before creating a connection from it.

the damage could already be done by the time user has a connection instance, since the connection creation involves a handshake and "Hello" message being sent out to the peer.

Even if you did Connection::system instead of Address::system I don't follow how a handshake "hello" message would be a security risk when using a memory-safe language like Rust.

[complexspaces]

However, I'm still wondering if it's even possible to verify the address? If that is easily possible (I understood that's what you claimed in your previous comment?) then we should just do it ourselves, w/o even involving the user/programmer.

I mentioned this in my email previously, but there are some strategies 1P has adopted for path verification but its not anything confident enough in (when trying to analyze the entirety of how Linux filesystem permissions can work) that I would want to put it in zbus as a layer of protection for users. We are fine owning the risk as a company but I think its too whack-a-mole for FOSS 😅.

I think if people are doing security sensitive things with zbus, then they should be responsible for making this set of risk acceptances and functionality tradeoffs instead of making you responsible for upkeeping complicated security checks forever.

[zeenix]

I think if people are doing security sensitive things with zbus, then they should be responsible for making this set of risk acceptances and functionality tradeoffs instead of making you responsible for upkeeping complicated security checks forever.

I agree but I feel like this is a lot more inline with my suggestion and the libdbus maintainer's conclusion. Having said that, I'm fine with telling people to verify the address (instead of ensuring their env is clean in general) if we can give them some clue how to achieve that (some link to documentation perhaps?).

[complexspaces]

Do you have a rough idea what documentation you'd like to see there? I'm not opposed to your idea of doing both here.

[zeenix]

My idea was simply that we tell people to ensure their environment is clean and why so.

[zeenix]

as for this specific comment of mine:

I'm fine with telling people to verify the address (instead of ensuring their env is clean in general) if we can give them some clue how to achieve that (some link to documentation perhaps?).

You seem to have a very good idea of strategies to verify the address so I'll leave this part to your capable hands. I just don't want people to read a non-actionable but specific warning.