19 ms·
Google broke a conditional statement that verifies passwords on Chrome OS
- wil421 5y agoCan the link be updated to the one below? The link above goes directly to the comments section and collapses the article, ?comments=1 is the culprit. https://arstechnica.com/gadgets/2021/07/google-pushed-a-one-character-typo-to-production-bricking-chrome-os-devices/ https://arstechnica.com/gadgets/2021/07/google-pushed-a-one-...
- stalfosknight 5y agoHN won't let me edit the URL. :/
- wil421 5y agoDang or someone else will probably see it. I didn’t think you’d be able to do it yourself.
- dang 5y agoFixed now. Thanks!
- dolni 5y agoIf you've been in tech for a while, this is one of those things that makes you sigh and shake your fist at "move fast and break stuff" as a strategy.
- throwntoday 5y agoThat's more about challenging the status quo not bad project management.
- ipsum2 5y agoGoogle's philosophy isn't "move fast and break stuff". It's probably the opposite - "move slow and be careful". These things happen.
- exikyut 5y ago"Product moved so slowly it ceased to be noticed by management, who accidentally moved resources elsewhere"
- pjmlp 5y agoWe need more lawsuits for faulty deliverables and less "reboot and hope it works again" attitude.
- contravariant 5y agoWait so are the devices bricked or not? The title says bricked but as far as I can tell people just can't log in temporarily until the fix is rolled out.
- penagwin 5y agoIt's a soft lock for sure, given it requires a software update to use you could likely consider this a soft brick too. It's not a hard brick to your point.
- sneak 5y agoNo, they are not bricked. The term has become ridiculously diluted in practice to mean "hard, breaking bug".
- dang 5y agoOk, we've done our bit to counter brick dilution in the title above.
- SquareWheel 5y agoAppreciated. Bricked used to mean utterly irreparable. The term certainly has seen dilution, and not for the better.
- Traster 5y agoI bet the post-mortem for this is going to be fun. How this wasn't covered by multiple unit tests, a code review and a roll out strategy is.... impressive.
- sneak 5y agoBitwise & versus logical && is a classic, right up there with an assignment in a comparison (when an equality check is intended), = for ==. That this was missed is pretty surprising, given that it's Google and the stakes involved in the encryption/key management code in a secure platform device. I wonder if we'll even get a postmortem, as this simply cannot happen unless several someones all Seriously Fucked Up simultaneously. - code error (expected, humans are fallible) - review error (less expected, this is the kind of thing code review exists for) - static analysis / linting check failure - integration test failure (interactive login no longer works on this build) This is a massive, overlapping fuckup. Heads should probably roll here, especially given that when you opt in to ChromeOS as a user, Google now has "one job" (DFIU).
- james_in_the_uk 5y agoStraight up negligence.
- brundolf 5y agoEspecially since it's so easy to lint for. This isn't some cross-file memory mishandling bug, it's a single token that's being used where it shouldn't be
- mdoms 5y ago> Heads should probably roll here, I disagree. It's obviously a major balls up by multiple people, but at the end of the day this is an organizational failure. You don't fire people for this, you learn from it and fix the organizational holes that caused it.
- sneak 5y agoSomeone is responsible for the organizational units and their processes. By that logic, almost no individual failure is a firing offense. Many people in the organization are responsible for ensuring that the processes themselves overlap in ways that exclude the possibility of failures like this. I'm talking about the meta-failure in management's oversight of the processes. There is belt person, and there is suspenders person, and there is a third person who makes sure there are both belt person and suspenders person on staff and that they don't go on vacation at the same time. Somewhere above the team lead and below the CEO there is someone who didn't do their job.
- mouzogu 5y agoCouldn't this be caused by a bad merge? Not sure how Google manages their merging and release process but there are conceivably several stages where a bad merge can occur and go unnoticed - possibly at a stage close to release and after testing.
- tmp_anon_22 5y agoAt an established company merging is an incredibly incredibly locked down process. There are usually multiple barriers of automation and people, entire release teams who all have to be stoned at the wheel to miss anything odd getting through.
- DelightOne 5y agoOr maybe a big automated refactor going too far? I wonder whether they run all tests on those.
- fouric 5y agoFrom the article: > The line should read "if (key_data_.has_value() && !key_data_->label().empty()) {" but instead of "&&"—the C++ version of the "AND" operator—the bad update used a single ampersand, breaking the second half of the conditional statement. Someone correct me if I'm wrong, but this seems like it wouldn't be possible in a language that didn't conflate boolean values with bitvectors. Edit: I was wrong[1]. [1] https://news.ycombinator.com/item?id=27923785 https://news.ycombinator.com/item?id=27923785
- CobrastanJorji 5y agoForgive my poor C++, but has_value() sounds like a bool, and empty() sounds like a bool. Why wouldn't "bool1 & bool2" work correctly?
- kevinmgranger 5y agoIf I had to guess, the second expression depends on the first being true. So if `has_value()` is false, perhaps `key_data_->label()` is undefined behavior.
- dataflow 5y agoDereferencing via key_label_-> has undefined behavior when !key_label_.has_value(), so the compiler treats that case the same as if key_label_.has_value(). Or to put it another way, the compiler reasons that key_label_ must have had a value (since you dereferenced it), and thus optimizes out the check for `has_value()` (since clearly it was unnecessary).
- fouric 5y agoAhhh, so it was a problem with lazy evaluation (and the lack thereof when using &), and not type coercion, like I initially thought.
- dataflow 5y agoYeah it has nothing to do with type coercion. I'm baffled a linter doesn't catch this though. & for bools should just be prohibited altogether. If people really want & for bool, they can just cast to an integer type and use & with that (which generates its own warnings appropriately).
- Animats 5y agoThe reason this happened is that it wasn't followed by "Google hit by 143 defective product lawsuits".
- kevincox 5y agoI don't think they could possibly be sued for this. They would just be responsible for fixing or refunding the product. In this case they issued a "fix". It is the same as when a car is defective. They don't get sued, they do a recall and fix it.
- perl4ever 5y ago>They don't get sued, they do a recall and fix it Are you kidding? Major (car, drug, you name it) companies have hundreds, if not thousands of lawsuits going at any given moment. Public companies will sometimes list major lawsuits in their annual report, but most of them don't get mentioned because the amount at stake is trivial for them.
- encryptluks2 5y agoYou have to remember, pretty much anyone can sue anyone... doesn't mean that they'll be successful.
- perl4ever 5y agoI don't have statistics on that, but I find it implausible that there would be hundreds or thousands of lawsuits against a typical corporation if say, only a couple % won or got a settlement.
- encryptluks2 5y agoI find it hard to believe that people are successful over lawsuits like this. Pretty much every product comes with an agreement that by using the device that you agree to it.
- SahAssar 5y agoJust reading the title it sounded very much like "Single Point of Failure: The (Fictional) Day Google Forgot To Check Passwords": https://www.youtube.com/watch?v=y4GB_NDU43Q https://www.youtube.com/watch?v=y4GB_NDU43Q
- jedimastert 5y agoI love Tom's speculative fiction talks. He tells such a good story.
- zinekeller 5y agoFortunately (or not, depending if you have a Chromebook and store files locally there), the inverse has happened and it locked up your book (fail-deadly versus fail-safe).
- utf_8x 5y ago> fail-deadly versus fail-safe Speaking of... https://www.youtube.com/watch?v=TUKQfMFKfjk https://www.youtube.com/watch?v=TUKQfMFKfjk
- 0x0 5y agoThis actually happened for real with Dropbox about 10 years ago: https://techcrunch.com/2011/06/20/dropbox-security-bug-made-passwords-optional-for-four-hours/ https://techcrunch.com/2011/06/20/dropbox-security-bug-made-...
- cmeacham98 5y agoThat exact news article appears in the video.
- secondcoming 5y agoEdit: Someone has explained the issue below I don't understand how that broke anything? The C++ codegen for && and & is the same: bool good(bool a, bool b) { return a && b; } good(bool, bool): mov eax, edi and eax, esi ret bool bad(bool a, bool b) { return a & b; } bad(bool, bool): mov eax, edi and eax, esi ret [0] https://godbolt.org/z/84MMqTehs https://godbolt.org/z/84MMqTehs
- taeric 5y agoThe short circuiting was preventing undefined behavior on the second expression. That is, yes, it is the same with booleans. Now try with expressions.
- formerly_proven 5y agoThe code is: key_data.has_value() && !key_data_->label().empty() key_data is probably something like an std::optional, so short-circuit evaluation guards the dereferencing of the optional. If you write this with a binary and, no short-circuit evaluation can happen, so undefined behavior ensues. I wanna point out that std::optional, in the typical C++ stance, has a "I know what I'm doing" API and a "Humans make mistakes API": optional->... is UB if the optional is empty, optional.value()... raises an exception. Arguably, in code like this you probably only want to use the latter kind of API. (The pattern of "operator can UB, equivalent method asserts pre-condition" is very common in the C++ standard library)
- jeffbee 5y agoCorrect, key_data_ is a base::Optional<KeyData>. Chromium's base::Optional is very similar to std::optional, but predates c++17.
- bialpio 5y agoExcept that Google uses C++ with exceptions disabled - IIUC, the throwing code would effectively behave like a call to std::terminate(), I haven't looked at the code to know if it would actually be a preferred behavior in this case. Source: I'm a Chrome contributor.
- jeffbee 5y agoThis is clearly a typo that happened to stomp some logic that is not covered by a test. The review 3002282 includes the bug, which deletes one of the & from a && operator. It says it was cherry-picked from review 2994464, which does not change that line. So, slip of the backspace key plus poor testing.
- tyingq 5y agoYeah, the biggest thing to me here is that there's no "login succeeded" unit test, integration test, manual test, etc. Whatever customer logged in first was the first person to actually run the code.
- black_puppydog 5y agoThat's not "bricking" as the title suggests. Sorry, word police incoming. :P
- maweki 5y agoIf there is such a process as unbricking, then the title is right. If becoming a brick is irrecoverable, then you are right. If you can't log into a device, it is a paperweight.
- snvzz 5y agoWhat it means is: They push crap to users with no testing. Testing would have caught this.
- schemescape 5y agoMaybe pushing updates to users is their test process? I doubt they're the only ones doing this, sadly.
- dodobirdlord 5y agoSince the issue is caused by an undefined behavior optimization, possibly the tests ran on a different optimization level or on a different architecture target? Would still be surprising, but less so than there not being tests at all.
- tpoindex 5y agoThere is a policy that would probably catch these types of errors - Chromeos developers should eat their own dog food, i.e., developers should be required to use a Chromebook on the "dev" or "beta" channels as their daily machines. Sure, use a cloud VM to get work done, but login, email, browsing, etc. done on the Chromebook. Google itself has fine machines for this purpose.
- deleted 5y ago[deleted]
- skinkestek 5y agoThe company who no longer recognizes doublequotes nor their own verbatim operator? The one who flags chess videos? Closes decade old accounts without any kind of explanation? Absolutely believable. Wouldn't surprise if this too is related to relying on their Artificial Stupidity^HIntellingence systems.
- iamcreasy 5y agoThe article says the problem bricked their laptop - but I was under the assumption that you can create a non-google account/guest account on Chrome OS to use the device.
- lmilcin 5y agoUmm... no automated tests? Junior engineers responsible for security of half of the world without proper code review?
- tpmx 5y agoHalf of the world? We're talking primarily north-american school kids. https://trends.google.com/trends/explore?q=chromebook https://trends.google.com/trends/explore?q=chromebook
- sellyme 5y ago>security of half the world Chrome OS has less than 2% market share.
- baud147258 5y agoI heard elsewhere that its market share is closer to 10% (and that it's now the second most popular desktop OS). 2% would be considering mobile OS too, I think https://www.maketecheasier.com/chrome-os-tops-macos-2nd-most-popular-os/ https://www.maketecheasier.com/chrome-os-tops-macos-2nd-most...
- tuxoko 5y agoI haven't touched C++ in forever, but as a C programmer it looks insane that you can do dot operator and -> operator on the same variable and not cause compiler error...
- foota 5y agoThe latter is the dereference operator, it's commonly overridden for single element container types.
- tyingq 5y agoI'm baffled that there's no test for this, manual or otherwise, before this rolls out to production. That you can change ChromeOS code in the login path, and the very first time it's exercised is by an end customer.
- dboreham 5y agoThis has happened so many times in recent years to high profile services and OSes. You'd think it'd be a test case.
- o8r3oFTZPE 5y agoHere is the original article from Android Police: https://www.androidpolice.com/2021/07/20/a-new-chrome-os-91-update-is-breaking-chromebooks-like-a-bull-in-a-china-shop/ https://www.androidpolice.com/2021/07/20/a-new-chrome-os-91-... The reason for the failed "update" was another Chrome 0-day that relies on the Google's Javascript engine: https://www.androidpolice.com/2021/07/16/another-day-another-major-chrome-zero-day-here-is-how-to-keep-yourself-protected/ https://www.androidpolice.com/2021/07/16/another-day-another... Curious whether they release the details of how this 0-day works after it is fixed, so we can see the mistake they made putting users at risk.
- cromka 5y agoSo what it means is that someone could potentially introduce another 0day while patching a 0day because the code reviewing of those escapes the scrutiny of a regular process.
- CapriciousCptl 5y agoThe bug, which used a single & in an if statement instead of a double && reminds me of this one from years ago-- https://lwn.net/Articles/57135/ https://lwn.net/Articles/57135/ In that case it was a single = versus a double == but also in if statement.
- rileytg 5y agoshocking there is not test that caught this
- harry8 5y ago(1 && 2) != (1 & 2) first both non zero so true and evalutes to non zero. Second 0b01 bitwise and 0b10 gives 0b00 which is obviously zero (or false).
- timvisee 5y agoRust would have prevented this.
- pornel 5y agoRust's Option can't be simply dereferenced causing UB, so `val.is_some() & val.kaboom()` doesn't compile. Rust doesn't have implicit conversion between booleans and integers, so a problematic `true & 2` does not compile either. Rust does allow `true & true`, but booleans are strictly 0 or 1 in value, so you wouldn't get a wrong result, apart from eager evaluation. There's needless_bitwise_bool clippy lint for it.
- timvisee 5y agoYes! While the other way around an accidental `&&` when `&` would have failed as well.
- jhasse 5y ago> Rust's Option can't be simply dereferenced causing UB, so `val.is_some() & val.kaboom()` doesn't compile. I guess the equivalent of `operator->` would be `unwrap()`. It isn't UB, so this bug couldn't happen in Rust indeed. > Rust doesn't have implicit conversion between booleans and integers, so a problematic `true & 2` does not compile either. That isn't what's happening here as both `key_data_.has_value()` and `!key_data_->label().empty()` return bool.
- smcl 5y agoSo what happens to people who are affected by this? Are they just out of luck and have to find another device or try to unbrick their chromebook somehow?
- throwawaycuriou 5y agoEDIT: Per the Android Police article, looks like there is a way to update now without powerwashing. Disregard the below paragraph. If they stored any data locally it's lost unless Google releases a way to allow a computer in this state to repair itself. Fortunately for many users the ChromeOS convention is to keep important things stored on the cloud. But that's not much comfort to those who chose to have experienced data loss due to this. The description of 'bricking' in this case is unwarranted. Their device is fine following a 'powerwash', essentially a factory reset.
- jasonvorhe 5y agoReading stuff like "heads gonna roll" in the comments is kind of shocking to me. Have you never screwed up in your job? How long have they shipped a secure Linux to a couple of hundred different devices simultaneously, while improving security, fixing critical bugs soon and delivering a pretty great user experience since they first launched Chrome OS? Sure, were soft bricks before, critical fixes introduced other bugs, duh. The same happens on Windows, macOS, Linux distributions - we're all human, we make mistakes, we try to learn from each mistake. Don't give them a hard time. They already were in panic mode already.
- nameless912 5y agoYup. My second or third month at my first job, I accidentally knocked out internet access for thousands of people for a couple of hours because I deleted something in production rather than in dev. It was terrifying and I was sure I would be fired. Rather than being canned, of course, the team managing the tool that allowed me to delete the resource learned the following: 1) people should not be able to delete things without confirmation in production 2) it should be very obvious which environment you're in 3) normal humans shouldn't have access to production except for operations folks. That's the value of blameless post mortems.
- arzeth 5y agogcc and clang warn about if (a = b) and recommend if ((a = b)) gcc and clang don't warn about if (a & b) and don't recommend if ((a & b)) That doesn't make sense.