Fix invalid shadowsocks ciphers crashing the app#10284
Conversation
aca5811 to
7c78e43
Compare
|
🚨 End to end tests failed. Please check the failed workflow run. |
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils reviewed 7 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on rablador).
ios/MullvadRustRuntime/MullvadConnectionModeProvider.swift line 11 at r1 (raw file):
import MullvadTypes public func initAccessMethodSettingsWrapper(methods: [PersistentAccessMethod], validShadowsocksCiphers: [String])
I wonder, could we add a unit test here that would've crashed before and wouldn't crash now?
Further, is there any risk in calling into rust to fetch the valid ciphers here instead of receiving the validShadowsocksCiphers argument? Feels a bit whack to pipe it all the way through.
Further, maybe it'd be best to do the filtering of the access methods in a function that is separate from all the low-level bit twiddling of pointers?
rablador
left a comment
There was a problem hiding this comment.
@rablador made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pinkisemils).
ios/MullvadRustRuntime/MullvadConnectionModeProvider.swift line 11 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
I wonder, could we add a unit test here that would've crashed before and wouldn't crash now?
Further, is there any risk in calling into rust to fetch the valid ciphers here instead of receiving the
validShadowsocksCiphersargument? Feels a bit whack to pipe it all the way through.Further, maybe it'd be best to do the filtering of the access methods in a function that is separate from all the low-level bit twiddling of pointers?
- We could I think. Let me have a look.
- No risk, but it boils down to the same discussion we had some time ago when the new cipher functionality was first introduced. Do we simply fetch it when needed (basically statically) or do we inject it "traditionally" from some shared origin.
- Sure
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on rablador).
ios/MullvadRustRuntime/MullvadConnectionModeProvider.swift line 11 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
- We could I think. Let me have a look.
- No risk, but it boils down to the same discussion we had some time ago when the new cipher functionality was first introduced. Do we simply fetch it when needed (basically statically) or do we inject it "traditionally" from some shared origin.
- Sure
Well, for testing purposes, providing a list of valid ciphers makes sense, so I retract that specific ask.
Sending an invalid shadowsocks cipher to the Rust layer causes a crash. This PR fixes this by validating the ciphers before initializing stored custom access methods.
This change is