no-verify-upload silently nullifies security benefits of append-only mode

[lgommans]

What should rest-server do differently?

The advantage offered by --append-only mode is nullified by disabling hash verification of uploads.

• One solution would be to make the options incompatible, i.e. exit if the user tries to do both at the same time. If they really understand what they are doing, they could still compile the server with this check removed.
• Another solution that occurred to me while writing this (at 3am local time with 5 hours of sleep last night... potential bad idea up ahead) is to add a check in saveBlob (and perhaps in other calls as well) to see whether the target file already exists. Perhaps regardless of append-only mode, as it seems the client just shouldn't do this. If we go this route, someone should first check if this truly solves the problem in a secure way.

Note that I did not try setting both options as I can't compile the project (it tries to pull over half a gigabyte of garbage for one, then discovers I need a different compiler version). I did look through the pull request for no-verify (which came after append-only) to see if it checks this in any way and it doesn't seem to me like this option combination is currently checked for.

What are you trying to do? What is your use case?

It's not me, it's them! If I'm not mistaken, an attacker can overwrite your data with empty or corrupt files by calling saveBlob on existing objects. This is typically prevented with the verification that seems to be on by default (judging by the option documentation). The user should be aware that these two options don't work together.

(Edit: perhaps I should elaborate on the public disclosure of a security-relevant bug: this isn't released yet and the no-verify option is discouraged in an obvious way. I do not think anyone would currently use this in production so I didn't want to demand fd0's private attention. It's also an easy check we can just add before a next release. /edit)

Did rest-server help you today? Did it make you happy in any way?

Having had a short look at rest-server's implementation: so long as you are careful with the forget hammer it seems solid, so I'm quite happy about that! I'd also like to write some documentation about this because currently the forget risk is not mentioned in server documentation (there is a terse mention in restic's threat model documentation, though), and it would also be useful to document how this important feature works given the very minimal changes in its implementing pull request (at first glance, one would think that there being no check in saveBlob is a major oversight (it's not), though saveConfig is still on the line given that restic doesn't seem to be able to recover from an overwritten config without writing code yourself), but... one step at a time. (Kinda documenting my to-do list here, don't mind me... *whistles*)

[wojas]

add a check in saveBlob (and perhaps in other calls as well) to see whether the target file already exists.

The check is here: https://github.com/restic/rest-server/blob/master/repo/repo.go#L552-L556

You are right that you should not use --append-only without hash verification in a production environment, but this is mostly because of the risk of corrupt data blocking the future upload of correct data.

The less safe option still has its uses, e.g. for home environments with slow NAS appliances, where it still prevents an attacker from overwriting or removing any existing blobs. It also prevents user mistakes.

Also note that the upload hash checks are a recent addition and rest-server supported --append-only for a long time without it.

[lgommans]

The check is here: https://github.com/restic/rest-server/blob/master/repo/repo.go#L552-L556

Oh! That's pretty stupid of me. I did think of this alternative solution while writing the ticket, but it did not even occur to me to check whether that is implemented, I only checked the solution I originally thought of. Nice, that resolves this!

One of the next things I was going to worry about was saveConfig since it also has no append-only mode check, but I see this is also already solved similarly: there totally is a file exists check. (And besides, it would technically be possible to restore without it, according to fd0.)

Excellent, that only leaves the forget problem that works relative to the latest (and attacker-controlled) snapshot according to the documentation. Not yet sure how to approach that one, but it's something for the main restic repository anyhow.

Thanks!

[lgommans]

<removed duplicate post> the form errored "you cannot comment at this time", so I retried with dev tools open; now my reply pops up twice (but the closure only once of course). Interesting.