5 ms·
The discussion on race conditions at the end is an important one, and IMO the bugfix is a bandage at best: the notion of anything accessing the “current” object
by btown 4y ago
The discussion on race conditions at the end is an important one, and IMO the bugfix is a bandage at best: the notion of anything accessing the “current” object after any kind of delay, especially in an event handler, when there is any chance the thing is not a singleton, is a recipe for disaster. In this case, dismissing the “current” security code screen was a supported API surface and that should set off all the red flags.
Of course it’s annoying to have to track the identity of “our screen” and bind that identity to event handlers, or make it accessible with context etc. But it’s necessary in anything remotely security-adjacent.
(And never assume anything is a singleton unless you add a breadcrumb comment for someone who might change that assumption on a different team!)
- dshpala 4y agoAnd of course fix includes magic SecurityMode.Invalid value, which makes dismiss() behave like it did before. I'd look very hard at places that use SecurityMode.Invalid.
- krajzeg 4y agoAgreed. The fixed logic, at least judging by the commit message, still feels very shaky on correctness grounds ("if we are dismissing something that doesn't seem to be right, ignore it"). Since they're rewriting code and changing method signatures anyway, I would prefer they got rid of the notion of "currently visible screen" and made sure that all dismiss() calls have a unique pointer or token pointing to what exactly is being dismissed. If this was my codebase, their approach would give me all sorts of bad vibes about additional problems lurking deeper. The whole process and the nature of the fix doesn't inspire a lot of confidence in the security of Pixel/Android in general.
- izacus 4y agoSo you'd go out and refactor a major security sensitive component (which dates to time before your career most likely) in a span of a single month for an emergency security patch deadline? That doesn't inspire a lot of confidence in your risk assesment and decision making. I'd do what Google did: rollout a patch that addresses the immediate danger and then backlog proper refactors over time.
- gjsman-1000 4y agoI don't think that is as much of an issue as the ridiculous process he had to go through. Think about that first security researcher. You literally found a Screen Unlock bypass (should be Priority #1, right?) - and Google just went and put fixing it on the backburner. If they will put something like that on the backburner, what else are they ignoring? It isn't confidence-inspiring. Edit: Also, knowing Google, what are the odds of your full refactor? "Temporary" fixes become permanent fixes quickly.
- izacus 4y ago> Edit: Also, knowing Google, what are the odds of your full refactor? "Temporary" fixes become permanent fixes quickly. Hahah, I wish that was only Google :D
- richardfey 4y agoCould have been sold for up to 300k or more on the black market.
- pmontra 4y agoMaybe it was an already well known exploit. After all this was a duplicate and Google was sitting on it. Two people found it and reported it to Google. Why not a third one, and sold it?
- tehlike 4y agoHahah it can go both ways. You can have 2 major rewrite over 3 years or you can have a new temporary-became-permanent bug fix.
- krajzeg 4y agoTheir fix included a similarly large refactor, they just used the "security screen type" as a newly introduced parameter instead of something unique to the screen instance. I do agree that in the real world, sometimes you have to settle for a less-than-ideal solution. I hope my post reads less like "those people are idiots", which was not my intent, but more like: this specific fix isn't ideal, and knowing this type of code is live in a device doesn't fill me with confidence, even if I can understand reasons for why it was done that way.
- cr4nberry 4y agoThis just sounds like you're prematurely optimizing for additional security screens getting added. Maybe that's not on the table atm? Still odd that they took so long to change a couple method signatures and write a couple test cases
- avianlyric 4y agoThey already have multiple security screens, and a demonstrated critical bug with security screen confusion. Not sure how this is premature optimisation.
- cr4nberry 4y agobecause if the number of screens is small and there are few tiers (only 2), passing an identifier around could be overkill sounds to me like it's an optimization for introducing more tiers than what there are
- strix_varius 4y ago> the number of screens is small and there are few tiers (only 2) Making this kind of assumption, when there are no such guards in the system itself, is exactly what leads to security issues. If the system enforced two named singletons as security screens, so it was impossible to .dismiss() the wrong thing, then sure. But that's not how the system is, and assuming that "the number of screens is small" and "there are only 2 tiers" without enforcing that assumption with code is pretty much how the original bug was introduced.
- roywashere 4y agoSince they are dismissing the Lock Screen _type_ (SIM, PUK, Pin) and not the instance, a logical example for where this might go wrong is if you have dual SIM. Then again, worst case you dismiss the incorrect SIM Lock Screen. That will not give you full unlock and also the ‘wrong’ SIM will still not work
- adrianprestamo2 4y ago
- adrianprestamo2 4y agorecursive comment
- adrianprestamo 4y agorecursive comment recursive
- kudokatz 4y ago> The fixed logic, at least judging by the commit message, still feels very shaky on correctness grounds This was my experience as a dev on a team at Google for a few years. I saw a LOT of exceedingly lax treatment of correctness in the face of concurrency. There have even been multiple decisions I've seen to guess at how to fix concurrency bugs and just say "well, looks good to me, let's see if it does anything." It's par for the course, and folks get (got? =P) paid handsomely for doing it.
- joezydeco 4y agoAm I reading this right? This reads like the presence of a UI element holds the unlock state of the phone?
- izacus 4y agoNo, not exactly, but Android is old and gnarly enough that a lot of components don't have a clear Model/View separation you'd expect in modern codebases.
- DonHopkins 4y agoThe good old "Model / View / Confuser" paradigm.
- bongobingo1 4y agoAndroid was invented before MVC?
- moffkalast 4y agoWell it was designed around Java, so definitely before common sense was invented.
- btown 4y agoWell, the UI element’s dismissal is what signals the system that holds unlock state. And the problem was that multiple other systems would automate the dismissal of the current UI element… without checking whether it was the appropriate UI element they expected to be there!
- oneplane 4y agoI would indeed expect something more robust like a security state machine where not all states can transition to any other state freely. The UI shouldn't even have a say in what transitions are allowed.
- nerpderp82 4y agoThe other nice quality if your code is in a state machine is that it can verified by a model checker. You might like this post on statig, an HSM library for Rust https://old.reddit.com/r/rust/comments/yqp2cq/announcing_statig_hierarchical_state_machines_for/ https://old.reddit.com/r/rust/comments/yqp2cq/announcing_sta...
- doorman2 4y agoThe Rx model works nicely. Rather than trying to model state transitions, you model "scopes of work." So if you have something like an event listener, you would tie it to the active unlock session. When that session ends, you dispose of that "scope of work" and all work associated with it would immediately stop.
- zitterbewegung 4y agoGoogle invests a great amount of money in their project zero team but does anyone know if they have a specific red team that is dedicated for Android ?
- mrb 4y agoAs a former Google Information Security Engineer I can say: I don't know, because the Android security organization is completely separate from Google's security organization. It's something I always found odd, as it created some redundancy between both organizations, lack of homogeneous processes, etc
- saagarjha 4y agoYes. See for example: https://i.blackhat.com/USA-22/Wednesday/US-22-Karimi-Google-Reimagined-A-Phone.pdf https://i.blackhat.com/USA-22/Wednesday/US-22-Karimi-Google-...
- bsder 4y agoYeah, I suspect this fix isn't going to hold for very long at all now that everybody knows to look at it. If Google isn't readying a more extensive fix right now, they're going to get pwned shortly.