Hackers Exploited 0-Day, Not 2018 Bug, to Mass-Wipe My Book Live Devices

Via: Ars Technica:

Last week’s mass-wiping of Western Digital My Book Live storage devices involved the exploitation of not just one vulnerability but a second critical security bug that allowed hackers to remotely perform a factory reset without a password, an investigation shows.

The vulnerability is remarkable because it made it trivial to wipe what is likely petabytes of user data. More notable still was that, according to the vulnerable code itself, a Western Digital developer actively removed code that required a valid user password before allowing factory resets to proceed.

My read on this is that Western Digital is going to face massive liability claims. Likely a huge class-action suit is in the making already. This will not ease the pain of all those folks who lost their data but will cost the company a lot of money. There are forensic tools to recover this lost data but the process is slow and expensive and never recovers everything.

a reply to: machineintelligence

a Western Digital developer actively removed code that required a valid user password before allowing factory resets to proceed.

Hm okay I am a beginner in programming and looked at the code.

function post($urlPath, $queryParams = null, $ouputFormat = 'xml') [
// if(!authenticateAsOwner($queryParams))
// [
// header("HTTP/1.0 401 Unauthorized");
// return;
// ]

First, why do they check a negative login response from that function? The ! negates the return result. So if the function returns true, it's negated to false and the code inside the brackets is never executed. Even more, if for some reasons the function "post" is never called, this indicates that then the user already has all the rights, but will cancel them if the login comes back wrong. This looks like a way of writing the code allowed for easy commenting out.

Analog, this would be like a door that's always open put if you want to go through, it locks up if you do not have a key. Wouldn't it be wiser to program a closed door and check if we open it for the ones with the correct credentials?

I am so confused now, the course I take, they teach me to back engineer every program that checks credentials or does file operations I write, for so called exploits. Keep it simple and straightforward and then try to disturb it.

Second, why is $quaryParams set to null to be checked by the authenticateAsOwner function? I don't get it at all.

a reply to: ThatDamnDuckAgain

Yeah. Western digital #ed up really bad with this one.

I am so confused now, the course I take, they teach me to back engineer every program that checks credentials or does file operations I write, for so called exploits. Keep it simple and straightforward and then try to disturb it.

Second, why is $quaryParams set to null to be checked by the authenticateAsOwner function? I don't get it at all.

There's not much to get...every company's got that one coworker that doesn't have a clue what they're doing and manages to get by...until they # up terribly.

a reply to: dug88

I begin to think that this is not the real code. There are typos in the variables names ("ouput") and [] used for wrapping the functions execution code, what language is this? It is not C++ or C#.

originally posted by: ThatDamnDuckAgain
a reply to: machineintelligence

a Western Digital developer actively removed code that required a valid user password before allowing factory resets to proceed.

Hm okay I am a beginner in programming and looked at the code.

function post($urlPath, $queryParams = null, $ouputFormat = 'xml') [
// if(!authenticateAsOwner($queryParams))
// [
// header("HTTP/1.0 401 Unauthorized");
// return;
// ]

First, why do they check a negative login response from that function? The ! negates the return result. So if the function returns true, it's negated to false and the code inside the brackets is never executed. Even more, if for some reasons the function "post" is never called, this indicates that then the user already has all the rights, but will cancel them if the login comes back wrong. This looks like a way of writing the code allowed for easy commenting out.

Analog, this would be like a door that's always open put if you want to go through, it locks up if you do not have a key. Wouldn't it be wiser to program a closed door and check if we open it for the ones with the correct credentials?

I am so confused now, the course I take, they teach me to back engineer every program that checks credentials or does file operations I write, for so called exploits. Keep it simple and straightforward and then try to disturb it.

Second, why is $quaryParams set to null to be checked by the authenticateAsOwner function? I don't get it at all.

Because authenticateAsOwner would return true if authenticated, so if it returns false you can see it sets header to 401 Unauthorized.

a reply to: ThatDamnDuckAgain

If this code was not commented, the first check is to see if the user is authenticated. If not, it writes a response to the request header.

The $queryParams = null is setting a default value for that variable. This is done to prevent NullException errors. If nothing is passed as an argument, it assigns it a value of null for checks further down the chain.

Looks to me like a function that was started and never finished, thus never implemented

a reply to: ThatDamnDuckAgain
It's PHP, and it looks correct to me in the article with [] < Apparently this site is sanitizing the curly braces into brackets

a reply to: ThatDamnDuckAgain

It's PHP. Honestly, i think the devs just didn't care. The my books were considered end of life by that point. It seems like the password authentication code was commented out at some point, probably when they stopped supporting the mybooks.

Then at some point, a hacker gained access to actual mybook server and changed the PHP scripts to be accessible by only their password.

Western digital knew about this in 2018.

nvd.nist.gov...

They just didn't care.

originally posted by: Tekner
a reply to: ThatDamnDuckAgain
It's PHP, and it looks correct to me in the article with [] < Apparently this site is sanitizing the curly braces into brackets

Oh you are correct, I checked the article! My mistake.

It's still looking strange, I will bring this example up in the course, to see what they have to say about it.

a reply to: ThatDamnDuckAgain

Here's a pastebin with the full code. It's a little more clear.

paste.debian.net...

a reply to: dug88
Got distracted by the door bell, was about to answer your other post.

Thank you

I looked at it but the function seems to be hidden away in a different class file. I want to understand why they pass null into the authenticate function. I know that I can pass null into a function and check for it to modify the behavior.

Like when I have two functions doing almost the same but one requires a second parameter that the other doesn't, I spare myself the typing work and use one function for both purposes. I pass null as that parameter and check for it in the function itself.

Then the function "knows" what path it needs to take.

a reply to: ThatDamnDuckAgain

My guess is maybe because they commented out the actual authentication code.

Here

function get($urlPath, $queryParams=null, $ouputFormat='xml')[
// if(!authenticateAsOwner($queryParams))
// [
// header("HTTP/1.0 401 Unauthorized");
// return;
// ]

and here, this is the function in your other comment about halfway down the file. the backslashes mean the code is commented out. There is no function. It doesn't check for authentication, that was the vulnerability and the problem.

function post($urlPath, $queryParams=null, $ouputFormat='xml')[
// if(!authenticateAsOwner($queryParams))
// [
// header("HTTP/1.0 401 Unauthorized");
// return;
// ]

The first block is supposed to get a password from the user, the second block posts it, but there is no password. There's just nothing because they're both commented out and take null as parameters. It will just always authenticate. It wasn't a mistake, it was done intentionally, again probably when they stopped supporting the device.

a reply to: dug88

I know, the // or */ /* initiates comment lines or sections

That's why I am so confused why they would write it like that. Not knowing the syntax correct, I would have used

if(authenticateAsOwner($quaryParams) //why t f is this set to null in the function constructor?
[
// do deleting stuff
]else
[
//middle finger up,
]


Add: I see that my issue in understanding is an intrinsic factor of dickery.


Add2: So you say it was done by intent and even if it had not been commented out, it would always authenticate?

a reply to: ThatDamnDuckAgain

I'm guessing because wherever those functions are called you're supposed to provide the parameters. Those are the function definitions not where the functions are called. It's just initializing the variable to null in case you don't pass any parameters and it still has old garbage left in memory.

It'd be something like

int addnumber(int x, int y = 3)[
return x+y;
]

addnumber(3); //will produce 6
addnumber(3,6); //produce 9
addnumber(); //will produce 3 or error out depending on the language and whether integers are default initialized with 0.

a reply to: dug88

Lol I think we have a communication issue but that's no doubt my fault.

addnumber(3); //will produce 6
addnumber(3,3); //produce 9
addnumber(); //will produce 3 or error out depending on the language and whether integers are default initialized with 0.

Like my example above, I actually do this and didn't get scolded yet.

Like
addnumber(int num1, int num2)
[
if(num2!=null) [ return num1+num2;]
else [ return num1+num1]
]

In this case, if I do not pass a second parameter, or pass a null as a parameter, it will just add num1+num1. Of course I would check num1 to be != null too. At this point I start to think about performance and wonder what the compiler will do when I write "return num1*2". Could it be faster because of bitwise addition stuff, I stepped out mentally when that was explained.


...
removed because it's better this way
...

originally posted by: machineintelligence
There are forensic tools to recover this lost data but the process is slow and expensive and never recovers everything.

Western Digital is offering data recovery for free to affected customers, from your link:

Update 6/29/2021, 9:00 PM: Western Digital has published an update that says the company will provide data recovery services starting early next month. My Book Live customers will also be eligible for a trade-in program so they can upgrade to My Cloud devices. A spokeswoman said the data recovery service will be free of charge...

Western Digital stopped supporting the My Book Live in 2015

If they stopped supporting those devices in 2015, they must be way out of warranty by now, right?
Also if people were using those for backups, they should still have the original files that were being backed up, and can trade in their mybook devices for the more recent MyCloud devices, WD has a trade-in program.

If I lost data this way I suppose I wouldn't be happy, but I always keep a backup somewhere so the data wouldn't be lost. I guess not everybody does that, but if their hard drive has been out of warranty for 5 years, they are already running a big risk of losing their data if it's not backed up, even with no exploits. Drives can just fail for any number of reasons at any time but especially when they are 5 years or more out of warranty.

a reply to: ThatDamnDuckAgain

So...$QueryParams I'm guessing is a string. By initializing $QueryParams as null, you're ensuring that the function is never run with garbage data inside.

Example, $QueryParams could hold the previous input from another session, if you called post and forgot to actually supply a $QueryParams it could potentially use the old data, by initializing it to null it will just crash, well it should, but again, the function is commented out so it doesn't matter at all.

If you call the function and supply the parameter, the null is overwritten by the arguments you supply.

It is good practice to initialize variables to null if there's a chance garbage data could be supplied that causes undefined behaviour.

Commenting out the authentication part is a monumental stuff up but by now the people who wrote the code and the management who authorised it will be long gone.

With anything these days you should make the code prove that the input is valid before doing anything other than throwing a wobbly but i suspect someone was doing some work and disabled authentication to save the typing in of passwords so they could test it quicker but forgot to re-enable the code and just pushed to live and then went straight onto the next job.

Least some lawyers will be getting new solid gold cars from this debacle.

a reply to: dug88

Example, $QueryParams could hold the previous input from another session, if you called post and forgot to actually supply a $QueryParams it could potentially use the old data, by initializing it to null it will just crash, well it should, but again, the function is commented out so it doesn't matter at all.

It is good practice to initialize variables to null if there's a chance garbage data could be supplied that causes undefined behaviour.

Is this (old data around) because PHP is used where there are many calls simultaneous and every browser / user shares the same instance on the machine that PHP is executed on?

That's, apart from the live parsing, the only difference I am able to spot between the two in usage.

a reply to: ThatDamnDuckAgain

There's probably no reason for it other than the programmer thought it was a good idea. It doesn't really matter because the functions shouldn't be called without arguments. There's a lot of useless things like that in programming that exist mostly just to stop you from messing up. It's pretty much the whole concept behind unit testing.

In the end, it's just safer to explicitly initialize it as null than not. Also, I don't know much about PHP, maybe it requires you to initialize variables with a value.

Some languages will not let you declare a variable without initializing it to some value, even null.

Mostly so you don't get problems like you get in C with trying to use uninitialized variables.