MAC limitations

[ahmedcharles]

Given an unencrypted key foo with value "23" (toml), then the following changes are allowed by the MAC.

foo.a = "23"

foo.a = "2"
foo.b = "3"

foo = 23

This doesn't work with encrypted keys, mostly, because they store the path, but in some cases, even that doesn't protect changes:

"to:ot" = "secret"

to.to = "secret"

They both have the same keys and since there is only one value, the MAC doesn't change.

Fixing these would make it not backwards compatible with SOPS. They could be fixed by including the type/length of the value in the MAC and by including the length of the key into the additional data, instead of a delimiter.

[gibbz00]

Hmm, if I remember things correctly, changes to MAC are only allowed if mac_only_encrypted is set to true.

The path only becomes relevant as the input for additional data when during value encryption, or when it changes the order in which values are hashed. (Should be documented in the concepts chapter of the book.)

But yes, because of how the path for how the additional data is computed, there's no difference between "to.to" and "to:to". Excellent catch. Had not thought of this myself.

Adding the type/length of the value in the MAC should because of all of this not really be relevant. As these are to separate checks. (Given that I understood you correctly). Or in other words; this is already done when mac_only_encrypted it's default false.

What do you mean by "including the length of the key into the additional data, instead of a delimiter."?

Solving the "to:to" and "to.to" issue could be done by adding another field/MAC in the metadata that SOPS doesn't check. Similar to how I had idea about computing an additional MAC over active integration keys to prevent against manual removal without rotating the secret data key.

[ahmedcharles]

For the field issue, the change would be here (perhaps in another MAC for compatibility with SOPS):

https://github.com/gibbz00/rops/blob/main/crates/lib/src/file/map/key_path.rs#L7

If that were a Vec<u8> instead, this could become

self.0.extend(&(other.len() as u32).to_le_bytes());
self.0.extend(other.as_bytes());

For the MAC over unencrypted values, you would change:

https://github.com/gibbz00/rops/blob/main/crates/lib/src/file/map/value/core.rs#L61

so that it would hash the type and length before the value, so for strings you could have:

const STRING_TYPE: u8 = 0;
RopsValue::String(string) => {
    let mut v = Vec::new();
    v.push(STRING_TYPE);
    v.extend(&(string.len() as u32).to_le_bytes());
    v.extend(string.as_bytes());
    Cow::Owned(v),
}

I didn't compile these, so there are probably some minor issues, but hopefully it illustrates the problems?

[gibbz00]

Cool, gotcha. Clicked having slept on it on what you mean by the MAC being indifferent to some things. (It only cares about byte order, after all.)

I'm opening up a separate issue as I suspect this should be two separate feature additions.

[gibbz00]

Do you think we should just add one MAC that catches both manipulation changes?

[ahmedcharles]

Well, the conceptual ideal would be to have a MAC which hashes something like deterministically encoded cbor. If using an implementation based on this idea, it would allow any change which didn't change the meaning but would disallow changes which did. Right now, swapping the ordering of values in a map changes the MAC but doesn't change the meaning.

[gibbz00]

Yeah, map ordering is also something that should not matter, but currently does. Same can't be said for lists though.

I'm not too familiar with CBOR, less so with the deterministic encoding. Are you suggesting than these 3 issues (path : manipulation, changing fields that result in same MAC, and MAC being determined by map key-value order), could all be solved by something similar?

[gibbz00]

I think we want to keep compatability with SOPS, so if we could add one metadata MAC that tackles all of this, then that would be great.

[ahmedcharles]

Yes, if the hash were determined by something similar to deterministically encoded cbor, then you could use that for the MAC and solve all three. That's one of the use cases for deterministically encoded cbor. Obviously, cbor is just one possible implementation, I just pointed it out because I'm familiar with it.

[gibbz00]

Ahh, ok, starting to see where you're getting at. Encode with cbor or any other format that is deterministic/care's only about properties that matter, then hash the encoded output? That would be really elegant. Removes the need for ROPS to take into account all the edge cases we've so far mentioned and probably some more.

[ahmedcharles]

Indeed, that is the idea, logically. There are nuanced questions about efficiency, but I suspect that for typical SOPS/ROPS file sizes, efficiency doesn't matter. :)

[gibbz00]

Awesome. I currently don't have the time to implement this any time soon, unfortunately. I would on the other hand be positive to any incoming PR that provides a solution 😊

Yeah, performance has not been a priority. I would guess that most time for normal usage is spent on I/O, which is pretty much beyond my control. Also, prioritizing performance over "correctness" in security critical applications makes me a bit nervous 😅