SlugSecurity / ucsc-ectf-2023

UCSC's design and implementation for MITRE's eCTF 2023
MIT License
0 stars 1 forks source link

Fix pairing PIN security vulnerability - [merged] #85

Closed eggroll-bot closed 1 year ago

eggroll-bot commented 1 year ago

Merges pairing-security-fix -> main

This security vulnerability is due to the fact that the pairing PIN response delay is only done when the PIN is wrong. This merge request makes it so that there is always a delay.

eggroll-bot commented 1 year ago

requested review from @jzhan357

eggroll-bot commented 1 year ago

requested review from @MelonShooter and removed review request for @jzhan357

eggroll-bot commented 1 year ago

In GitLab by @MelonShooter on Mar 5, 2023, 23:16

Commented on docker_env/util_no_std/src/runtime.rs line 120

Don't fully qualify these things and don't specify the trait the ptr() associated function comes from. It's unnecessary if you import the trait

eggroll-bot commented 1 year ago

In GitLab by @MelonShooter on Mar 5, 2023, 23:16

Commented on docker_env/util_no_std/src/runtime.rs line 152

Why do you grow in here when you allocate in the From implementation for RuntimePeripherals.

Also, you need to be careful here because to use a static mut without synchronized access, you must ensure that it is only ever accessed on a single thread throughout its lifetime.

eggroll-bot commented 1 year ago

In GitLab by @MelonShooter on Mar 5, 2023, 23:16

Commented on fob/src/pairing.rs line 68

This tuple literal is incredibly difficult to read, separate the creation of the HibTimer out into another variable

eggroll-bot commented 1 year ago

In GitLab by @MelonShooter on Mar 5, 2023, 23:16

Commented on fob/src/pairing.rs line 140

Put this stuff into a separate function without the while !pin_cooldown_timer.poll() {} and call it here. Then, run that line after the function returns. This way, you can ensure that it waits for the timer on all exit paths much easier.

eggroll-bot commented 1 year ago

added 5 commits

Compare with previous version

eggroll-bot commented 1 year ago

changed this line in version 3 of the diff

eggroll-bot commented 1 year ago

changed this line in version 3 of the diff

eggroll-bot commented 1 year ago

added 1 commit

Compare with previous version

eggroll-bot commented 1 year ago

In GitLab by @MelonShooter on Mar 6, 2023, 02:14

Commented on docker_env/util_no_std/src/runtime.rs line 319

You must add dsb() (dmb() is probably fine too) after this to ensure all threads see this because RuntimePeripherals is Send. Technically we're in a single-threaded safe environment but it's not safe otherwise. This isn't necessary b/c the Arc is allocated in the thread.

eggroll-bot commented 1 year ago

You don't need a memory barrier. The arc is created in the same function.

eggroll-bot commented 1 year ago

resolved all threads

eggroll-bot commented 1 year ago

In GitLab by @MelonShooter on Mar 6, 2023, 02:23

approved this merge request

eggroll-bot commented 1 year ago

added 1 commit

Compare with previous version