-
Notifications
You must be signed in to change notification settings - Fork 141
Rust Installer #291
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
base: main
Are you sure you want to change the base?
Rust Installer #291
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i really like this approach, since afaict it means we can ship the entire release as a single platform-specific binary. just have a few questions/comments in my review
5a5e45f
to
0e6353c
Compare
0fcf380
to
fd05825
Compare
Ok(adb_device) | ||
} | ||
|
||
async fn test_rayhunter(adb_device: &mut ADBUSBDevice) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the benefit of not requiring adb forward ...
(or to be connect via rndis or wifi to the orbic) but it has the downside of leaving the Orbic in a state where a user may not be able to see the web interface after the install without additional steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The adb library doesn't have a forward function so I came up with this as a compromise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is something we'll want to chew on. It's probably fine to switch users to having to connect to the Orbic over wifi or usb to test whether it's running, but right now this isn't really doing anything different than pgrep rayhunter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little more than that, but I agree a more robust test would be preferable.
It would help reviewing if, since this is replacing |
It does the same thing as EFForg#272 but only installs necessary files. Installation happens entirely over the network so there is no dependency on ADB. Currently can be used like this: 1. cargo build --bin rayhunter-daemon --target armv7-unknown-linux-gnueabihf --release --no-default-features --features tplink 2. cp target/armv7-unknown-linux-gnueabihf/release/rayhunter-daemon dist/rayhunter-daemon-tplink 3. cargo run --bin installer -- install-tplink
fd05825
to
47fa724
Compare
Done. |
Found an exploit that requires fewer HTTP requests and can be run without auth.
47fa724
to
23d2d2d
Compare
Made several of the requested changes except for those I've replied to with questions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still a draft PR? it seems to me the main thing missing is to remove the old installer and adjust the README
Yeah, I guess it's not a draft anymore. I'll update the README and remove the old installer soon. Also waiting to hear back on a few comments. |
@@ -1 +1,2 @@ | |||
/target | |||
*.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once these & the older comments are addressed I'm excited to merge this.
} | ||
} | ||
}, | ||
Err(RustADBError::IOError(e)) if e.kind() == ErrorKind::ResourceBusy => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run installer orbic
on an already rooted device, I get the ORBIC_BUSY error message and the install fails.
However, it doesn't have to. serial
calls device.detch_and_claim_interface()
@ https://github.com./EFForg/rayhunter/blob/main/serial/src/main.rs#L140 to work around needing to kill the adb daemon.
This is a regression that I think we have the tools to avoid, since we're using your fork of adb_client.
} | ||
|
||
async fn wait_for_adb_shell() -> Result<ADBUSBDevice> { | ||
const MAX_FAILURES: u32 = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only waits for 10 seconds, but in other cases we wait at least 30 seconds for a reboot.
let mut adb_device = force_debug_mode().await?; | ||
let serial_interface = open_orbic()?.ok_or_else(|| anyhow!(ORBIC_NOT_FOUND))?; | ||
print!("Installing rootshell..."); | ||
std::io::stdout().flush()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that if https://doc.rust-lang.org/std/io/trait.Write.html#tymethod.flush fails (which isn't a critical event on its own), the installer will stop and report an error. .ok();
seems like a better fit.
optional: We also have this print-flush pattern -- seems like a good place for a macro (echo!()
?) to improve readability.
Finally, and this is so unimportant, but since we're here: it'll look better if we print!("Installing rootshell... "); ... println!("done");
with the trailing space after the ellipsis so error messages look better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would writing to stdout fail? I would expect the installer to stop on broken pipes or similar, just how all these print/println
statements abort/panic on IO error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was imagining a scenario like a full disk that the output was being piped to, or a program popen'd the installer then closed just the installer's stdout pipe. If that will cause the program to crash at every print!/println!, then you're right, there's no point in trying to make the flush
es robust.
name: installer-${{ matrix.platform.name }} | ||
path: target/${{ matrix.platform.target }}/release/installer${{ matrix.platform.os == 'windows-latest' && '.exe' || '' }} | ||
if-no-files-found: error | ||
|
||
build_release_zip: | ||
needs: | ||
- build_serial_and_check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- build_serial_and_check | |
- build_rayhunter_check |
This PR adds a now merged Rust installer that supports both the Orbic and TP-Link devices. It currently depends on my fork of adb_client: https://github.com./gaykitty/adb_client/tree/nusb