12 ms·
Alleged Mt.Gox code leaked on IRC node by Russian Hacker
- cognivore 13y agoApologies in advance to the PHPers out there, but... PHP?!
- TazeTSchnitzel 13y agoMagicalTux is a known lunatic who wants to write everything in PHP. Which isn't too bad of an idea, but he, for example, hacked together an SSHd in PHP and immediately put it into production use: http://blog.magicaltux.net/2010/06/27/php-can-do-anything-what-about-some-ssh/ http://blog.magicaltux.net/2010/06/27/php-can-do-anything-wh... Essentially he is a bad programmer and he has Not Invented Here (fixed, thanks jey) syndrome, both of which are terrible attributes for someone coding a Bitcoin exchange.
- deleted 13y ago[deleted]
- jey 13y agoI think you meant NIH ("Not Invented Here"), not NIMBY ("Not In My Backyard").
- TazeTSchnitzel 13y agoOops, yes I did indeed mean Not Invented Here syndrome. Thanks.
- stormbrew 13y ago.. Well.. at least he didn't write the crypto primitives himself in PHP.
- brianpgordon 13y agoHe wrote the DH key exchange himself. I didn't look at the implementation but someone commented before that it's vulnerable.
- mukyu 13y agoI actually wrote that comment back in the day. I was rather snarky and unhelpful it seems. The code for the sshd does not seem to be there anymore, but from memory: it did not check if the number sent by Bob was 0, 1, or any any other groups that would make it easy solve the discrete logarithm problem. I don't think it bothered to check the primes either. [1] I think there was also something wrong with the signature checking (padding not checked maybe?). Altogether it seemed like you could easily MITM connections made to the server, but I don't think I ever tried. It was a perfect example--to me at least--of why you should not spend a trivial amount of time reading about crypto on Wikipedia and then writing crypto code.
- olssonm 13y agoJust for the sake of argument – any reason why not PHP?
- shalmanese 13y agoFor a bank? I can think of lots of reasons. * You're exposed to any upstream bugs in the language and interpreter * Dynamic typing and silent errors mean you're far more likely to make a mistake and not realize it * It's missing a lot of battle tested libraries and frameworks * It's PHP
- Gigablah 13y agoMaybe someone would like to post a link to the veekun article for the umpteenth time.
- ColdHawaiian 13y agoI'm assuming that you're referring to "PHP: a fractal of bad design"[1]? [1]: http://me.veekun.com/blog/2012/04/09/php-a-fractal-of-bad-design/ http://me.veekun.com/blog/2012/04/09/php-a-fractal-of-bad-de...
- mappu 13y ago> You're exposed to any upstream bugs in the language and interpreter How would you mitigate that, and how is the mitigation process any different for PHP?
- someone13 13y agoNot to say whether PHP is in fact a good idea or not, but at least in a statically-compiled language, once you've got a "working binary", however you accomplish that, it will presumably continue to function that same way indefinitely. Obviously barring OS-level problems, of course :)
- nodesocket 13y agoWould you consider Python and Ruby strongly typed? Because I happen to know two very large payments company using them. However if that is indeed their code, it makes me cry and cringe, even though PHP is my strongest language and I'm proud to admit it.
- ing33k 13y agono apologies needed , but will be better if you can add more details on why you think using PHP is bad (for Mt.Gox ).
- dschiptsov 13y agoIsn't it self-evident?) Having such a bad code in the worst language imaginable that eventually the site has been cracked by Russian punks. This will be a great case study.
- krapp 13y agoDo you believe that Mt.Gox would still have everybody's bitcoins if it had been written in Ruby or Python or Node.js?
- jl2035 13y agoIf there was the same people (person) behind it.. No.
- TazeTSchnitzel 13y agoOoh, floating-point arithmetic in software making financial transactions!
- deleted 13y ago[deleted]
- stinos 13y agoindeed. (int)round($info['balance'] * 100000000) such constructs are not a good sign. Quick glance over the rest of the code also confirms this. Way too much room for improvement (and that is a friendly way of saying it).
- makomk 13y agoThat's harmless and more or less unavoidable in this case. While the official Bitcoin client uses safe fixed-point arithmetic internally, the JSON API it exposes uses floating point all over the place. This isn't a problem so long as you correctly convert the data back to fixed point as soon as you get it before doing any other arithmetic on it, exactly as this code is doing. (All the numbers are guaranteed to be small enough that this isn't going to lead to rounding errors if done correctly.)
- JumpCrisscross 13y agoPardon my naïveté - why would floating point arithmetic be a red flag in transactional code?
- route66 13y agoIt's less about the transactions but the money. >> 3 * 1005.01 => 3015.0299999999997 This might get truncated to 3015.02 and might >> 3 * 1005.01 * 10000 => 30150299.999999996 turn into a real loss.
- brianpgordon 13y agoLike Superman 3!
- DonGateley 13y agoLooking at that code makes me yearn for Cobol.
- gnerd 13y agoI had a little peak and started seeing a few bugs but then I hit line 1009: > mail('mark@ookoo.org', 'BLOCK IMPORT ERROR', $e->getMessage()."\n\n".$e); I haven't done much digging around this issue (so perhaps someone can enlighten me on this) but I was curious at what was at that domain (since it seemed the email was going to Mark Karpeles). When I went to the staff page[1] I could see his company Mutum Sigillum LLC takes care of the administration for that IRC network. That same staff page used his nickname MagicalTux and the corresponding page[2] links to his "Professionnal PHP5 certification"[3] but that lists his name as "Robert Karpeles" and not Mark Karpeles. Does anyone know why this is? Did he change his name between 2006 and now? [1] http://en.wiki.gg.st/wiki/Staff http://en.wiki.gg.st/wiki/Staff [2] http://en.wiki.gg.st/wiki/MagicalTux http://en.wiki.gg.st/wiki/MagicalTux [3] http://www.expertrating.com/transcript.asp?transcriptid=1005930&submit=Submit http://www.expertrating.com/transcript.asp?transcriptid=1005... Edit: After more digging and looking at his test scores and code, I have come to the conclusion that this guy's skill level is not what he thinks it is. Here is my favourite: http://code.ohloh.net/file?fid=it28K0-Hdeyw2F2XDguAEU0ZSKI&cid=lrhV9kzrbr8&s= http://code.ohloh.net/file?fid=it28K0-Hdeyw2F2XDguAEU0ZSKI&c... Notice his fix to stop local file inclusion vulnerabilities in PHP, apparently he has never heard of a null byte.
- deleted 13y ago[deleted]
- groovylick 13y agoPlus just about 200 lines later there is a completely different email address to notify of a failed ssh connection. >mail('mark@tibanne.com,luke+eligius@dashjr.org', 'SSH connection to freetxn@'.$ip.' failed', 'Used ssh key 14a70b11-5f36-4890-82ca-5de820882c7f, but couldn\'t login to push those txs:'."\n".implode("\n", $el_todo));
- ojr 13y agoIt is impressive that PHP ran the largest Bitcoin exchange for a while and the main flaw happened to be a software hack than its ability to scale. If you think language X would be more secure and saved Mt Gox, the Russians would like to have a word with you
- gnerd 13y agoPHP is a tool and as any tool with a low learning curve it can be used by people with or without expertise. I think we certainly have had PHP built infrastructure that scales, but surely you can see why an order matching system should not be written in PHP. First of all, by doing it that way, the order matching system was coupled to the website, so now it makes perfect sense why the BitCoin price crashed after the DDoS attacks on MtGox. Because taking the website down meant taking the order matching down with it. No more trading. From what I am seeing, this is not a case of PHP being evil (although, would you really run mission critical systems with PHP? The execution model doesn't make sense in that world and if you think a set_time_limit(0) on a PHP script is the same as an actual daemon written in a robust language meant for that execution model, then I think we are in extreme disagreement). For me this is a case of a guy who's confidence was ahead of the reality. I'm sure in his mind a pacemaker running on PHP code is perfectly fine, and perhaps it might actually work for a while but that's just it, it will fail eventually (its a square peg in a round hole after all) and when it does it will be bad. We don't craft critical systems thinking of the best case scenario, we do it thinking of the worst and for my money, whatever happened at MtGox, its the worst case scenario. PHP in Facebook, yes, NYSE? FUCK NO.
- girvo 13y agoI love PHP. Yes, you read that right. And, I completely agree with you. Also, for the GP, PHP's shared-nothing architecture means scaling horizontally is actually pretty easy all things considered. I just wouldn't write a bitcoin exchange in anything that doesn't have static typing: PHPs gotchas could quite literally cause a massive loss of money.
- ramchip 13y ago
- Nilzor 13y agoWow. This code is pretty bad. I mean, it's bad for a college project. It's horrible for a company dealing with large sums of money. Some random red flags: - There's a class with the name of the application. (Issues: Scope, SRP) - There's a class with 1708 lines of code. (Scope) - There's a switch-case statement that runs over 150 LOC (readability, maintainability) - There's a string parsing function in the same class as transaction processing (Separation of concerns) - There are segments of code commented out (are they not using source control?) - There's inlined SQL (maintainability, security) - There's JSON being generated manually & inline (SoC, DRY) - There's XML being generated manually & inline (SoC, DRY) - To sum up function _Route_getStats($path): XML production, JSON production, file writing, business logic, SQL commands, HTTP header fiddling, hard coded paging limits, multiple exit points... The amount of refactoring needed here to bring this code up to acceptable quality is simply staggering.
- zedr 13y agoWhere can I get nilzor_lint.pl?
- easy_rider 13y agofor an MVP this is fine :D Seeing how long mtgox has been around this is absurd. 1700+ lines would spin my head pretty fast.. And inline SQL is non-forgivable.
- davidw 13y ago> And inline SQL is non-forgivable What do PHP folk typically do for a big, hairy query that isn't really doable via an ORM? Not that this is the case here, just curious. I haven't touched PHP for years.
- stingraycharles 13y agoIt's always better to use prepared statements, since it prevents SQL injection attacks. If you do not use neither an ORM or prepared statements, you either have to with trusted input, or need to sanitize that input really well (to the point it's often easier to just use prepared statements).
- tobehonest 13y agoNow that it has been shown the NSA to offensive attacks to discredit anti-government forces, could any of this mess be attributed to PSYOPS to discredit the whole cryptocurrencies?
- dschiptsov 13y agoPHP... What a farce.
- smutticus 13y agoAm I to believe that MtGox code that actually touched Bitcoin transactions was actually written in PHP? Is this some sick joke? I have used PHP on and off for years for many things. I'm not a PHP hater, but if it's not touching HTTP, it probably shouldn't be written in PHP.
- Aqueous 13y agoThere's nothing intrinsically wrong with that if you do it correctly. But this was not done correctly.
- seunosewa 13y agoI don't read or write PHP but the code looks quite neat and well-organized to me...
- McGlockenshire 13y agoIt's organized, but not well built. Previous comments: https://news.ycombinator.com/item?id=7332372 https://news.ycombinator.com/item?id=7332372
- mavdi 13y agoYou should really look into PHP then. Lots of like minded people with such organised and neat code. /s
- sasas 13y agoThe the leaked IRC chat logs [1] the leaker stated he was Serbian, not Russian. Is there additional information available? [1] http://www.reddit.com/r/Bitcoin/comments/1x93tf/some_irc_chatter_about_what_is_going_on_at_mtgox/cf99yac http://www.reddit.com/r/Bitcoin/comments/1x93tf/some_irc_cha...
- putakamo 13y agogfxmod sdt435436 ewrter r 345 23 r5w35wq43523 rw4t543523r5234rwetretyew456423 534y6456432t ertretyq34twer tety3452145 34t563425435345 235234 5345345
- putakamo 13y agogfxm0d 435rew fgaswr35rw4r tweqr45 3qtrfgrsetwqrtwerfeqw5243242342342 3vrwevrfeaerfwer wqer 3 rwr2342345r grtytrty54t643tet43523gfhfmnyty3 t43q werqw4r5w 43523 b23r432523