6 ms·

Agreed. 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 see

Agreed. 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 ago
So 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 ago
  I 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 ago
    Could have been sold for up to 300k or more on the black market.
    - pmontra 4y ago
      Maybe 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 ago
    Hahah 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 ago
  Their 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.
  - btown 4y ago
    Right? This was absolutely the "right" level of refactor for a hotfix, as the full refactor would introduce much more state management that could itself introduce bugs. And especially if behind the scenes there was a detailed audit of what things can currently access the current security screen, it would be fine for now. But I sincerely hope that in the postmortem, there would be a larger meta-discussion around code review practices and how something like this "global dismiss" became part of the API surface to begin with, and a sincere prioritization of a larger review within the backlog. Though with everyone on edge at this time in big tech, I doubt that ends up happening :(
  - bragr 4y ago
    >Their fix included a similarly large refactor Their change is hardly a big refactor. This includes all the new code, all the parameter changes everywhere the function is used, and two additional test cases. This is a tiny change. >12 changed files with 102 additions and 26 deletions. [1] https://github.com/aosp-mirror/platform_frameworks_base/commit/ecbed81c3a331f2f0458923cc7e744c85ece96da https://github.com/aosp-mirror/platform_frameworks_base/comm...
cr4nberry 4y ago
This 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 ago
  They already have multiple security screens, and a demonstrated critical bug with security screen confusion. Not sure how this is premature optimisation.
  - cr4nberry 4y ago
    because 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 ago
        Since 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 ago
  recursive comment
  - adrianprestamo 4y ago
    recursive 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.

izacus 4y ago

So 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 ago
I 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 ago
  Could have been sold for up to 300k or more on the black market.
  - pmontra 4y ago
    Maybe 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 ago
  Hahah 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 ago
Their 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.
- btown 4y ago
  Right? This was absolutely the "right" level of refactor for a hotfix, as the full refactor would introduce much more state management that could itself introduce bugs. And especially if behind the scenes there was a detailed audit of what things can currently access the current security screen, it would be fine for now. But I sincerely hope that in the postmortem, there would be a larger meta-discussion around code review practices and how something like this "global dismiss" became part of the API surface to begin with, and a sincere prioritization of a larger review within the backlog. Though with everyone on edge at this time in big tech, I doubt that ends up happening :(
- bragr 4y ago
  >Their fix included a similarly large refactor Their change is hardly a big refactor. This includes all the new code, all the parameter changes everywhere the function is used, and two additional test cases. This is a tiny change. >12 changed files with 102 additions and 26 deletions. [1] https://github.com/aosp-mirror/platform_frameworks_base/commit/ecbed81c3a331f2f0458923cc7e744c85ece96da https://github.com/aosp-mirror/platform_frameworks_base/comm...

cr4nberry 4y ago

This 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 ago
They already have multiple security screens, and a demonstrated critical bug with security screen confusion. Not sure how this is premature optimisation.
- cr4nberry 4y ago
  because 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 ago
      Since 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 ago
recursive comment
- adrianprestamo 4y ago
  recursive 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.