7 ms·

A Bad Privacy Bug

110 points by stilist 14y ago

biot 14y ago
The schema should really be: private bit not null default 0 If you use something like tinyint and have a range of values that are acceptable, bake this into the database with a check constraint: myfield tinyint not null default 0 check (myfield between 0 and 3) Regardless of how comprehensive your unit tests are, you should prevent as much bad data as you can at the DB layer.
vlucas 14y ago
Man, the more I see and experience things like this in MySQL, the more impressed (horrified?) I am at how it ever became popular. So glad I've made the switch to Postgres.
- ars 14y ago
  This isn't really a MySQL issue though. He used bin instead of int, and MySQL did exactly what he asked - stored the raw binary data as is.
  - drivebyacct2 14y ago
    Why are rows with 'y' in them being returned when he asked for that value equal to zero?
    - ars 14y ago
      No idea. I would expect it to convert the 0 to a binary format (since that's the format of the column) but it's not. It does raise warning 1292: Truncated incorrect DOUBLE value: 'y ' 1292: Truncated incorrect DOUBLE value: 'y ' (Yes, twice.) Which makes no sense to me.
8ig8 14y ago
I was affected by this. I received the email notification from Maciej and was offered compensation for the mistake. I declined the compensation and instead upgraded my account to include archival service. I did this partially because I've been considering the upgrade for a while, but mainly because I was so impressed by the way the incident was handled. Keep in mind that I don't have state secrets bookmarked in Pinboard. I'm just an anti-social type of user as their tag line notes and I appreciate how Maciej runs his business.
jrochkind1 14y ago
> So even though the row has (from our perspective) a non-zero value, MySQL sees it as matching zero. What? Why does MySQL return the row with 'y' in it (in a 'binary' column) when you ask for "where row = 0'? wtf is going on here? That seems to be the real culprit here. I assume it's MySQL's 'helpful' conversion routines again, but I'm just assuming, I don't understand the details.
tdavis 14y ago
As much as anything else, this is another great example of MySQL letting you shoot yourself in the face by default. I also love that you can save "y" to an integer field without complaint. It seems obvious that accurate data validation and comparison would be the cornerstone of a successful relational database, yet MySQL defaults fail to provide any domain-relevant value whatsoever.
- ars 14y ago
  bin is not an integer field. It's a binary field - it can save anything. And no, you can not save 'y' to an integer field, so your rant is misplaced.
  - mbrubeck 14y ago
    I think tdavis is referring to this example from maciej's post, where inserting 'y' into a tinyint field silently converts it to 0 instead of throwing an error: mysql> create table demo(int tinyint(1), bin binary(1)); mysql> insert into demo(int, bin) values ('y', 'y'); mysql> select * from demo; +------+------+ | int | bin | +------+------+ | 0 | y | +------+------+
    - ars 14y ago
      It's not silent: mysql> insert into demo values ('y', 'y'); Query OK, 1 row affected, 1 warning (0.01 sec) mysql> show warnings; +---------+------+------------------------------------------------------+ | Level | Code | Message | +---------+------+------------------------------------------------------+ | Warning | 1366 | Incorrect integer value: 'y' for column 'n' at row 1 | +---------+------+------------------------------------------------------+ If you want this type of warning to be treated as an error then adjust the setting: mysql> SET sql_mode = 'TRADITIONAL'; mysql> insert into demo values ('y', 'y'); ERROR 1366 (HY000): Incorrect integer value: 'y' for column 'n' at row 1
      - 14y ago
mbq 14y ago
So you browse "private" bookmarks of your users on a daily basis...
deleted 14y ago
[deleted]
rev087 14y ago
If only other companies would handle issues with this level of transparency...
PascalW 14y ago
Instead of automated test suites, I use checklists before deploying major changes, performing a series of actions (like creating and editing bookmarks) to make sure everything works as expected. Somehow I always shiver when I read stuff like this. Some manual smoke testing is never bad, but I really can't understand how you can feel comportable about not having automated test suites on (larger) projects. Stuff like this is so easy to catch in a unit or integration test.
- EvilTerran 14y ago
  Genuinely curious - if you find a project in that sort of condition, how do you go about fixing it? Are there respected guides to introducing proper unit and integration tests to established projects (that may not be structured amenably to them)?
  - xentronium 14y ago
    There were a couple Destroy All Software screencasts on that topic [1]. In a nutshell, you start by writing a lot of high level integration tests, which will serve you as some guarantee you don't screw up. Then you start refactoring, writing fine-grained unit tests along the way. Once you settle with code structure, you might want to start removing/rewriting test cases you wrote initially in case they seem redundant. [1] https://www.destroyallsoftware.com/screencasts/catalog/untested-code-part-1-introduction https://www.destroyallsoftware.com/screencasts/catalog/untes... and further parts 2-4. This screencast is done mostly in ruby.
    - kevinpet 14y ago
      I find introducing high level integration tests to be much harder than introducing lower level unit tests as code starts getting modified. In theory, yes, that's the way to go, but in practice, you probably can't get there without significant refactoring.
  - calpaterson 14y ago
    Essentially, my understand of best practice is to write high level functional tests for the features that appear to work and then use them to ensure there are no regressions as a result of your changes. Someone people even define legacy code as "code without tests". http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052 http://www.amazon.com/Working-Effectively-Legacy-Michael-Fea...
brlewis 14y ago
I respect maciej more for the candor about this incident. My wife had private bookmarks exposed, but I recommended she continue to use the service. These particular mistakes are not ones I would have made myself, but the important thing is that he's examining them intelligently and taking steps to prevent their recurrence.
hopeless 14y ago
I recently watched this video which is mostly about Postgres but also pokes some fun at MySQL's weirdness. I didn't realise how scary MySQL can be http://vimeo.com/m/43536445 http://vimeo.com/m/43536445
davidmp 14y ago
But I do this from my own Pinboard account, which has superpowers. Specifically, it lets me see private bookmarks on all accounts. And since I already see everything, I don't notice when everybody else starts to see everything, too. This is not okay.
- thaumaturgy 14y ago
  I'm a drooling Pinboard fanboy, so I'm probably cutting him more slack here than I would for other services. However: it's been obvious to me since the beginning that "private" in Pinboard-context just means, "this bookmark is not publicly available on my account page." I've had absolutely no expectation that it wasn't accessible by a site admin -- although that would be nice too. I can't think of a way to store retrievable data, like a bookmark, on his servers without leaving some way for him to access the data if he wanted to.
  - davidmp 14y ago
    I'm a paying user and this won't make me stop using the service, but there should be a reasonable expectation of privacy here. Obviously the database has to store this information in a retrievable way, but to expose it so carelessly on a regular basis is completely unnecessary.
  - gyardley 14y ago
    Yes, retrievable data is obviously accessible by a site admin somehow. But the way he's got things set up: 1) It's trivial for him to inadvertently see something deeply personal to someone just by browsing the 'recent' list or doing a search. UPDATE: I overstated this one - Maciej let me know by email that he can only access private data on the search / recent page if he intentionally masquerades a user. He can only inadvertently see private data when viewing individual user pages. 2) If his account's ever compromised (let's hope he's not reusing that password elsewhere!) then someone else gets that ability as well, accessible from any browser anywhere. It's one thing when you have to ssh into a server somewhere and do a SQL query to access someone's private information. It's another thing to set up your admin account so you're casually exposed to it. I like Pinboard's service too, but this isn't remotely cool.
overgard 14y ago
I don't think I saw it mentioned anywhere, but another thought that occurs to me is it might have been initially useful when he created the table to have the "private" field actually be called "public" instead. The idea being that if the field is likely to be initialized to a default, you want the safer default. (IE, in that case things would be created as accidentally private, rather than as accidentally public) Kudos to the author for the transparency. I actually have never used pinboard, but that blog post actually makes me more likely to use it rather then less.
themonk 14y ago
Implicit type conversion can be evil : http://bhati.livejournal.com/3352.html http://bhati.livejournal.com/3352.html

110 points by stilist 14y ago

biot 14y ago

The schema should really be: private bit not null default 0 If you use something like tinyint and have a range of values that are acceptable, bake this into the database with a check constraint: myfield tinyint not null default 0 check (myfield between 0 and 3) Regardless of how comprehensive your unit tests are, you should prevent as much bad data as you can at the DB layer.

vlucas 14y ago

Man, the more I see and experience things like this in MySQL, the more impressed (horrified?) I am at how it ever became popular. So glad I've made the switch to Postgres.

ars 14y ago
This isn't really a MySQL issue though. He used bin instead of int, and MySQL did exactly what he asked - stored the raw binary data as is.
- drivebyacct2 14y ago
  Why are rows with 'y' in them being returned when he asked for that value equal to zero?
  - ars 14y ago
    No idea. I would expect it to convert the 0 to a binary format (since that's the format of the column) but it's not. It does raise warning 1292: Truncated incorrect DOUBLE value: 'y ' 1292: Truncated incorrect DOUBLE value: 'y ' (Yes, twice.) Which makes no sense to me.

8ig8 14y ago

I was affected by this. I received the email notification from Maciej and was offered compensation for the mistake. I declined the compensation and instead upgraded my account to include archival service. I did this partially because I've been considering the upgrade for a while, but mainly because I was so impressed by the way the incident was handled. Keep in mind that I don't have state secrets bookmarked in Pinboard. I'm just an anti-social type of user as their tag line notes and I appreciate how Maciej runs his business.

jrochkind1 14y ago

> So even though the row has (from our perspective) a non-zero value, MySQL sees it as matching zero. What? Why does MySQL return the row with 'y' in it (in a 'binary' column) when you ask for "where row = 0'? wtf is going on here? That seems to be the real culprit here. I assume it's MySQL's 'helpful' conversion routines again, but I'm just assuming, I don't understand the details.

tdavis 14y ago

As much as anything else, this is another great example of MySQL letting you shoot yourself in the face by default. I also love that you can save "y" to an integer field without complaint. It seems obvious that accurate data validation and comparison would be the cornerstone of a successful relational database, yet MySQL defaults fail to provide any domain-relevant value whatsoever.

ars 14y ago
bin is not an integer field. It's a binary field - it can save anything. And no, you can not save 'y' to an integer field, so your rant is misplaced.
- mbrubeck 14y ago
  I think tdavis is referring to this example from maciej's post, where inserting 'y' into a tinyint field silently converts it to 0 instead of throwing an error: mysql> create table demo(int tinyint(1), bin binary(1)); mysql> insert into demo(int, bin) values ('y', 'y'); mysql> select * from demo; +------+------+ | int | bin | +------+------+ | 0 | y | +------+------+
  - ars 14y ago
    It's not silent: mysql> insert into demo values ('y', 'y'); Query OK, 1 row affected, 1 warning (0.01 sec) mysql> show warnings; +---------+------+------------------------------------------------------+ | Level | Code | Message | +---------+------+------------------------------------------------------+ | Warning | 1366 | Incorrect integer value: 'y' for column 'n' at row 1 | +---------+------+------------------------------------------------------+ If you want this type of warning to be treated as an error then adjust the setting: mysql> SET sql_mode = 'TRADITIONAL'; mysql> insert into demo values ('y', 'y'); ERROR 1366 (HY000): Incorrect integer value: 'y' for column 'n' at row 1
    - 14y ago

mbq 14y ago

So you browse "private" bookmarks of your users on a daily basis...

deleted 14y ago

[deleted]

rev087 14y ago

If only other companies would handle issues with this level of transparency...

PascalW 14y ago

Instead of automated test suites, I use checklists before deploying major changes, performing a series of actions (like creating and editing bookmarks) to make sure everything works as expected. Somehow I always shiver when I read stuff like this. Some manual smoke testing is never bad, but I really can't understand how you can feel comportable about not having automated test suites on (larger) projects. Stuff like this is so easy to catch in a unit or integration test.

EvilTerran 14y ago
Genuinely curious - if you find a project in that sort of condition, how do you go about fixing it? Are there respected guides to introducing proper unit and integration tests to established projects (that may not be structured amenably to them)?
- xentronium 14y ago
  There were a couple Destroy All Software screencasts on that topic [1]. In a nutshell, you start by writing a lot of high level integration tests, which will serve you as some guarantee you don't screw up. Then you start refactoring, writing fine-grained unit tests along the way. Once you settle with code structure, you might want to start removing/rewriting test cases you wrote initially in case they seem redundant. [1] https://www.destroyallsoftware.com/screencasts/catalog/untested-code-part-1-introduction https://www.destroyallsoftware.com/screencasts/catalog/untes... and further parts 2-4. This screencast is done mostly in ruby.
  - kevinpet 14y ago
    I find introducing high level integration tests to be much harder than introducing lower level unit tests as code starts getting modified. In theory, yes, that's the way to go, but in practice, you probably can't get there without significant refactoring.
- calpaterson 14y ago
  Essentially, my understand of best practice is to write high level functional tests for the features that appear to work and then use them to ensure there are no regressions as a result of your changes. Someone people even define legacy code as "code without tests". http://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052 http://www.amazon.com/Working-Effectively-Legacy-Michael-Fea...

brlewis 14y ago

I respect maciej more for the candor about this incident. My wife had private bookmarks exposed, but I recommended she continue to use the service. These particular mistakes are not ones I would have made myself, but the important thing is that he's examining them intelligently and taking steps to prevent their recurrence.

hopeless 14y ago

I recently watched this video which is mostly about Postgres but also pokes some fun at MySQL's weirdness. I didn't realise how scary MySQL can be http://vimeo.com/m/43536445 http://vimeo.com/m/43536445

davidmp 14y ago

But I do this from my own Pinboard account, which has superpowers. Specifically, it lets me see private bookmarks on all accounts. And since I already see everything, I don't notice when everybody else starts to see everything, too. This is not okay.

thaumaturgy 14y ago
I'm a drooling Pinboard fanboy, so I'm probably cutting him more slack here than I would for other services. However: it's been obvious to me since the beginning that "private" in Pinboard-context just means, "this bookmark is not publicly available on my account page." I've had absolutely no expectation that it wasn't accessible by a site admin -- although that would be nice too. I can't think of a way to store retrievable data, like a bookmark, on his servers without leaving some way for him to access the data if he wanted to.
- davidmp 14y ago
  I'm a paying user and this won't make me stop using the service, but there should be a reasonable expectation of privacy here. Obviously the database has to store this information in a retrievable way, but to expose it so carelessly on a regular basis is completely unnecessary.
- gyardley 14y ago
  Yes, retrievable data is obviously accessible by a site admin somehow. But the way he's got things set up: 1) It's trivial for him to inadvertently see something deeply personal to someone just by browsing the 'recent' list or doing a search. UPDATE: I overstated this one - Maciej let me know by email that he can only access private data on the search / recent page if he intentionally masquerades a user. He can only inadvertently see private data when viewing individual user pages. 2) If his account's ever compromised (let's hope he's not reusing that password elsewhere!) then someone else gets that ability as well, accessible from any browser anywhere. It's one thing when you have to ssh into a server somewhere and do a SQL query to access someone's private information. It's another thing to set up your admin account so you're casually exposed to it. I like Pinboard's service too, but this isn't remotely cool.

overgard 14y ago

I don't think I saw it mentioned anywhere, but another thought that occurs to me is it might have been initially useful when he created the table to have the "private" field actually be called "public" instead. The idea being that if the field is likely to be initialized to a default, you want the safer default. (IE, in that case things would be created as accidentally private, rather than as accidentally public) Kudos to the author for the transparency. I actually have never used pinboard, but that blog post actually makes me more likely to use it rather then less.

themonk 14y ago

Implicit type conversion can be evil : http://bhati.livejournal.com/3352.html http://bhati.livejournal.com/3352.html