search

hzrd149 / blossom

Blobs stored simply on mediaservers
The Unlicense
55 stars 6 forks source link

Add requirement for sha256 on upload #12

Closed hzrd149 closed 1 month ago

hzrd149 commented 1 month ago

Replace the requirement for the size tag with an x tag containing the sha256 hash of the blob being uploaded

This is much more secure and removes the possibility of an attacker using another users "upload authorization" event with a padded blob

Giszmo commented 1 month ago

The hash also allows you to skip the upload altogether if the file is already known. On the other hand, why drop the size? You might have authorization to upload files of 1MB but not of 2MB, so by advertising the size, the server could reject it at this point?

Why does the content contain the verb "upload" or "delete"? Wouldn't it be cleaner to contain the filename, only?

VnUgE commented 1 month ago

I think it's fair to assume the HTTP request would have content-length alongside the event headers. The sha provides data integrity so length would just be checked alongside. An http 100-continue could serve this purpose better.

I really like the idea of providing content integrity in the signed message. I was going to open an issue last week but I wrongly assumed you already considered that and had a reason not to.

Although does the the server really need to care about data integrity since we assume all clients verify blob integrity during a download anyway, or do we trust servers to verify integrity?

franzaps commented 1 month ago

Besides better security, including a hash also has the upside of skipping existing uploads, +1 @Giszmo

If it's fast enough on most devices for average uploads, I'd say yes.

Q: regarding size, were Blossom servers rejecting outsized files based on the auth event declared size or via request headers?

hzrd149 commented 1 month ago

regarding size, were Blossom servers rejecting outsized files based on the auth event declared size or via request headers?

Yes, at least my initial implementation. although as I mentioned before it was only there as a really simple security measure to make sure the uploaded blob matched the users auth event

Although does the the server really need to care about data integrity since we assume all clients verify blob integrity during a download anyway, or do we trust servers to verify integrity?

The clients can verify the hash on download, but its also good to verify it in the other direction. It can protect against man in the middle attacks. and also prevents an attacker or malicious server from re-using the users upload auth to upload other blobs (of the same size) to other servers

Why does the content contain the verb "upload" or "delete"? Wouldn't it be cleaner to contain the filename, only?

The content of the auth can be anything and should be a human readable explanation of what the auth event is intended to be used for. Its mostly for convenience so the user knows what they are signing

flox1an commented 1 month ago

Looks good. https://bouquet.slidestr.net/ is now already adding the x tag (with client-sdk 0.8). Also added an optional check to https://media-server.slidestr.net/

Having the hash before the upload allows us to improve the upload performance as well. Currently we store the file in a temp location, calculate the hash on the server and then upload to a blob storage. This leads to significant delay for larger files. When the hash is known beforehand the file can be directly uploaded to the final location in an optimistic way. We would only need to revert the storage when the hash does not match.

hzrd149 commented 1 month ago

If there are no objections to this Ill merge it tomorrow