Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pin lock - RFC #622

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Pin lock - RFC #622

wants to merge 6 commits into from

Conversation

surfdado
Copy link
Contributor

@surfdado surfdado commented May 1, 2023

This isn't meant to be merged yet, mainly posted it for people to comment on or test independently.

Basic idea is to introduce a simple method to prevent accidental writes to the wrong VESC by using a 4-digit PIN to write-lock your firmware. Unlike pairing it is actually enforced by the firmware, so rogue 3rd party apps cannot circumvent it. Also, a 4-digit PIN is easy to remember.

PIN is stored in EEPROM just like the Odometer, however this implementation does clear the PIN during firmware writes.

Functionality is fully described here:
https://pev.dev/t/new-feature-spec-pin-locking/815/4

Signed-off-by: Dado Mista <dadomista@gmail.com>
Signed-off-by: Dado Mista <dadomista@gmail.com>
@surfdado surfdado changed the title Pin lock - RFQ Pin lock - RFC May 1, 2023
When calling LOCK_STATUS we now also return another flag to indicate when
the pin is 0.

Signed-off-by: Dado Mista <dadomista@gmail.com>
Odometer in the app isn't entered in meters, so it's tricky to produce an exact match.
Check for match within 500m...
Signed-off-by: Dado Mista <dadomista@gmail.com>
LOCK_STATUS now also returns whether lock_on_boot is set to true
Bugfix in check-writelock function

Signed-off-by: Dado Mista <dadomista@gmail.com>
@Gabrielerusso
Copy link
Contributor

this is interesting for who uses ebike-scooters and leave them parked or use them in crowded areas where other people could be malicious.

@surfdado
Copy link
Contributor Author

this is interesting for who uses ebike-scooters and leave them parked or use them in crowded areas where other people could be malicious.

Agreed - in the Float Package for example we have a feature to disable the whole board. Combined with the PIN lock it would be impossible for a 3rd party to get the vehicle running again, without disassembling it and accessing USB or the debug pins.

@Relys
Copy link

Relys commented Aug 20, 2023

Great feature. First of all my question is why is the official UUID pairing even a thing if it's client side? That renders it completely pointless and it should just be entirely removed if that's how it functions IMHO.

Second question, is there a timeout of how many pin combinations can be attempted before requiring a power cycle (i.e. preventing a brute-force attack)?

@surfdado
Copy link
Contributor Author

First question, idk - not my implementation. It was developed back when VESC Tool was the only tool/app available to prevent accidental pairing of the wrong remote.

Second, we can consider making it more hacker proof later, right now the aim is to keep it simple and idiot proof.

@Relys
Copy link

Relys commented Jan 17, 2024

I submitted a PR to @surfdado branch for this btw: surfdado/bldc#4

"Adds a new command to COMM_WRITE_UNLOCK_CMD where you can append the pin before the actual command and payload which will handle writelock unlocking/locking while handling the command. Calls the command processing function recursively like COMM_FORWARD_CAN. Fixed some vulnerabilities with stack overflow by limiting recursion depth (which was also in COMM_FORWARD_CAN) as well as fixing brute force attempts for writelock."

As bldc gets more popular in semi-commercial applications (such as the Floatwheel), I do think it's worth putting some time aside for discussion and development of some basic security features in this safety critical system.

@surfdado
Copy link
Contributor Author

I submitted a PR to @surfdado branch for this btw: surfdado/bldc#4

I'll check it out as soon as I get a chance!

@tonyt321
Copy link

tonyt321 commented Jul 3, 2024

would like this to be added officially would save a lot of confusion when connecting with multiple boards around

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants