search

scooterhacking / NinebotCrypto

An implementation of the NinebotCrypto protocol by majsi.
GNU Affero General Public License v3.0
42 stars 15 forks source link

Add better swift implementation #12

Closed LexNastin closed 8 months ago

LexNastin commented 8 months ago

All the implementations currently stem from the C# implementation by @majsi, and are difficult to read and maintain (although in his defense, that code was mostly automatically generated by a decompiler and was never intended for public use). The Swift implementation also has many faults due to being simple copy paste of the C# version, expecting behaviours that aren't present in the Swift language. There were also multiple mistakes the original author, making his implementation unreliable.

This new Swift implementation completely reimplements all the crypto code and replaces it with more secure functions from CryptoSwift and CryptoKit. It's easier to follow what it's doing and easier to modify as needed as it heavily simplifies on @majsi's original code.

Thanks :)

bastianpedersen commented 8 months ago

If this is tested and works as expected, I don't see any problem merging this in.

LexNastin commented 8 months ago

Yep, is tested and works as expected. Able to auth, encrypt, and decrypt successfully with no data loss or anything.

bastianpedersen commented 8 months ago

Have you also compared behavioral differences between the current version and this one, just to rule out anything weird going on?

LexNastin commented 8 months ago

The only ACTUAL behavioral difference is that the current one changes counter from 0 to 2 after the first message, but it appears that that isn't intentional and going from 0 to 1 is perfectly fine, so I made it like that in the new one. Aside from that it's functionally identical and matches the output of the current one :)

Also, this PR doesn't actually remove the current one, it just adds a new file BetterNinebotCrypto.swift.

LexNastin commented 8 months ago

Also, @nopbxlr said he'll validate my changes later today, so if you want you can leave it to him.

bastianpedersen commented 8 months ago

Alright, sounds good to me. Thank you for the contribution.

nopbxlr commented 8 months ago

The only ACTUAL behavioral difference is that the current one changes counter from 0 to 2 after the first message, but it appears that that isn't intentional and going from 0 to 1 is perfectly fine, so I made it like that in the new one. Aside from that it's functionally identical and matches the output of the current one :)

Wouldn't you believe it.. that's the same bug I found today! It only seems to cause issues on the G65/G2/F2 scooters and that's why I was looking into it. It makes communication very slow as you must always send messages twice for the counter to be ok and to get a reply. Funny timing!

robbiet480 commented 8 months ago

Thanks for this @LexNastin. I am a decent Swift developer but didn't understand C# and majsi wasn't available to walk me through the code during my translation.

Out of curiosity, what app are you building? Would love to beta test!

ArchGryphon9362 commented 8 months ago

Thanks for this @LexNastin. I am a decent Swift developer but didn't understand C# and majsi wasn't available to walk me through the code during my translation.

Out of curiosity, what app are you building? Would love to beta test!

Check out https://github.com/ArchGryphon9362/asu-app. Thanks for the curiosity. Your implementation was very useful anyways. Thanks for implementing it! The app is currently in rather early stages, but it’s getting there. I’m currently working on fixing Xiaomi specific authentication. It will be a dashboard app with SHFW setting and flashing capabilities. (i’ll be publishing a stripped, dash only app on the App Store later on too, and leaving the one with flashing and SHFW capabilities for sideloading only)

robbiet480 commented 8 months ago

Hell yeah. Why limit flashing/SHFW to side loading only?

lekrsu commented 8 months ago

Hell yeah. Why limit flashing/SHFW to side loading only?

Apple would by every ounce of their being not allow that on their store.

robbiet480 commented 8 months ago

Eh, speaking as someone that has constantly pushed the envelope of what's allowed in the store (I authored Home Assistant for iOS) I think you have a good shot of getting it in. Worth a submission to try at least.

lekrsu commented 8 months ago

I think you have a good shot of getting it in. Worth a submission to try at least.

Yeah sure I agree, would be awesome if they would. That would clear up the process for iPhone users quite a bit.

ArchGryphon9362 commented 8 months ago

Hell yeah. Why limit flashing/SHFW to side loading only?

It is against App Store policy to publish apps made for the purpose of modifying third party devices, and they often reject apps for simply using third party APIs without the third party’s consent.

Eh, speaking as someone that has constantly pushed the envelope of what's allowed in the store (I authored Home Assistant for iOS) I think you have a good shot of getting it in. Worth a submission to try at least.

I suppose I COULD try publish the unstripped version and see if Apple tries to shoot me down, but how risky would that be do you think?

P.S. Nice work on HASS iOS, great to have it :)

robbiet480 commented 8 months ago

If they say no it's not like you're going to lose your developer account. Just submit again stating in the release notes that you removed firmware functionality and you should be fine. Reach out to me elsewhere if thats not the case, Home Assistant has brought me some friends high up at Apple that can help with this kind of thing.

ArchGryphon9362 commented 8 months ago

If they say no it's not like you're going to lose your developer account. Just submit again stating in the release notes that you removed firmware functionality and you should be fine. Reach out to me elsewhere if thats not the case, Home Assistant has brought me some friends high up at Apple that can help with this kind of thing.

Thanks! I’ll try it out when the app is ready for that and will be sure to keep you in mind. Hope it goes well!

lekrsu commented 8 months ago

If they say no it's not like you're going to lose your developer account. Just submit again stating in the release notes that you removed firmware functionality and you should be fine. Reach out to me elsewhere if thats not the case, Home Assistant has brought me some friends high up at Apple that can help with this kind of thing.

Was there anything in particular that wouldn't initially allow Home Assistant on the store? Did anything have to be changed source-wise or was it up to convincing?