Conversation
Added in this commit: * TLS 1.3 requirement * A section about rate-limiting and replay protection * Nonces for each request submits data * sha256 file hashes in prepare-upload * A note about limiting transmissionId reuse * 429 errors to all routes for rate-limiting * Explicit max sizes for prepare-upload (max file size, max number of files, max total size) This commit also adds some more punctuation to stray sections.
README.md
Outdated
| ## 5. Rate-limiting and replay protection | ||
|
|
||
| ### 5.1 Rate-limiting | ||
|
|
||
| All routes are rate-limited per IP address, limiting the amount of requests a | ||
| single IP is allowed to make for each route. | ||
|
|
||
| If an IP address becomes rate-limited, its requests are ignored and the | ||
| error 429 "Too many requests" sent as response. | ||
|
|
||
| ### 5.2 Replay Protection | ||
|
|
||
| All routes that submit data are guarded against replay attacks by the inclusion | ||
| of a nonce. If a request contains a nonce that has been seen, that request is | ||
| regarded as invalid and is rejected. |
There was a problem hiding this comment.
I'm not sure about needing this section, but I wanted to at least write a little bit about the rate limiting and replay protection. Adding a new section about those was the simplest solution I could think of :)
We can discuss if the section is needed or how/whether to do the rate limiting / nonces
There was a problem hiding this comment.
Yes, it’s important to add rate limiting. let me investigate about how to add the rate limiting in the server
There was a problem hiding this comment.
Rate-limiting and 429 to all routes is a good idea I think we have many options that we can investigate and agreed on to add it?
There was a problem hiding this comment.
Sounds good! This is also something that we don't need to coordinate too much on. But like I mentioned in the call just now, if we for example rate-limit by recording the incoming (local) IP on a given route (like /upload) then that lets us limit some types of attacks that could use the rate-limiting as a denial of service attack (if we did not use the ip or hash(ip) in the key).
README.md
Outdated
| ### 4.2 File Upload | ||
|
|
||
| The file upload requires the sessionId, fileId, and its file-specific transmissionId obtained from /prepare-upload. | ||
|
|
||
| `PUT /api/v1/upload?sessionId=sessionId&fileId=fileId&transmissionId=transmissionId` | ||
| `PUT /api/v1/upload?sessionId=sessionId&fileId=fileId&transmissionId=transmissionId&nonce=random-uuid-number` | ||
|
|
||
|
|
There was a problem hiding this comment.
On fixing some stuff in desktop, I noticed that there is currently no good error to return when the receiver (server) cancels an ongoing transfer (the button "Stop transfer", see screenshot)
(UI looks funky because I have resized it awkwardly on my screen:)
There was a problem hiding this comment.
yes it’s better to add an error
On iOS, in the current implementation, when the receiver cancels an ongoing transfer, the server simply closes the connection
There was a problem hiding this comment.
Ah that's good to know! I'll see if I can mimic that behaviour in Desktop's frontend :)
|
Another one: also noticed that during the following flow there's no way (?) to pick up on it from a sender (iOS/Android)
|
based on team disucssions we decided: * TLS1.2 has to be possible due to Tella supporting android versions where TLS1.3 is currently not available * defer defining maximums for file size, transfer size, max file counts until priority fixes have been implemented
this commit introduces mTLS to Tella's P2P protocol. previously, only the sender was authenticating and verifying the receiver's identity. with the changes described in this commit, the receiver can now verify the sender's identity. since the two ends of the connection are now authenticating the identity of the other for each request after hitting /register, we say that the connection is mutually authenticated with "mutual TLS" (mTLS).
|
@dhekra-rouatbi @ahlem-jarrar commit 178807d describes the changes (as I perceive them) needed for mTLS. what do you think, is it unclear in any part? |
| |401|Invalid session ID| | ||
| |403|Rejected| | ||
| |429|Too many requests| | ||
| |500|Server error| |
There was a problem hiding this comment.
@cblgh Do you think that we need to implement 507 : Insufficient storage space now or let’s keep it for later ? We already support this case in the design
There was a problem hiding this comment.
@dhekra-rouatbi If you and @ahlem-jarrar have the ability to support it then I think we should add it now. Checking the available space on desktop should be close to straightforward
There was a problem hiding this comment.
I think we should have it, but for android I can do it for V2 release, we can get approval from @carohadad
3 participants
In this PR I have include changes intended to address findings of the security audit done in late 2025.
This PR is created so that we can discuss what protocol changes can be made in the short-term based on team workloads and on the security benefit of the given change. Some things we might have to strike from the PR because of workloads or discussions, but hopefully we have the space to make changes that further improve Nearby Sharing's security profile.
The PR is a work in progress, some things (like exact values for max file sizes) are undecided at the time of submission. We should agree on limits all together, as they have an impact on usability (smaller sizes limits the files that can be sent) but also on security (limits effect of attacks, enables server-side size validation).
Having the PR discussion history lets us also have a good way to find out "wait why did we decide on this again?"
cc @carohadad @dhekra-rouatbi @ahlem-jarrar
p.s. I also fixed up some punctuation and typos, and added a link to Tella's website.