4 ms·

After a quick peruse of their github repository I have to say there is some dodgy[1] stuff going on. As ever with PHP sites validation seems to be a mess. For

by orf 10y ago

After a quick peruse of their github repository I have to say there is some dodgy[1] stuff going on. As ever with PHP sites validation seems to be a mess. For example we've got a `escapeSupplierName` function[2] used in some places[3], but a different regex used when creating a suppier[4]. Variants of that regex also appear all over the code, why not put it in a single place? Properly escaping SQL injection is great (and something PHP apps seem to have a continual problem with), but why did you decide to roll your own framework for doing this? Are all of the well maintained, tested and great ones already available not suitable for some reason? It's just your app turns into a hard to check, inefficient[5] sludge, full of lots of code for handling framework related things like an ORM, caching[6] (that code is practically duplicated in a lot of places[7][8][9][10]), escaping[11], mime whitelisting[12] etc and not much about a CMS. This is where security bugs appear. 1. https://github.com/paragonie/airship/blob/eb4293aee5be59e329015520de7e0e44c3593ee6/tools/hangar/src/Command.php#L233-L237 https://github.com/paragonie/airship/blob/eb4293aee5be59e329... 2. https://github.com/paragonie/airship/blob/c4c9384d4d7860738d4965ed0be9f8a27c18e1ca/src/Engine/Bolt/Supplier.php#L77 https://github.com/paragonie/airship/blob/c4c9384d4d7860738d... 3. https://github.com/paragonie/airship/blob/c4c9384d4d7860738d4965ed0be9f8a27c18e1ca/src/Engine/Bolt/Supplier.php#L97 https://github.com/paragonie/airship/blob/c4c9384d4d7860738d... 4. https://github.com/paragonie/airship/blob/95af23ca782e8ecb1083d31f8614a7ca532e3207/src/Engine/Keyggdrasil/TreeUpdate.php#L354 https://github.com/paragonie/airship/blob/95af23ca782e8ecb10... 5. https://github.com/paragonie/airship/blob/63cf0661ba21cbb3f3bfff510854a716f4b5579b/src/Cabin/Bridge/Model/Files.php#L1055-L1062 https://github.com/paragonie/airship/blob/63cf0661ba21cbb3f3... 6. https://github.com/paragonie/airship/blob/master/src/view_functions.php#L646-L681 https://github.com/paragonie/airship/blob/master/src/view_fu... 7. https://github.com/paragonie/airship/blob/master/src/view_functions.php#L718-L736 https://github.com/paragonie/airship/blob/master/src/view_fu... 8. https://github.com/paragonie/airship/blob/master/src/view_functions.php#L491-L510 https://github.com/paragonie/airship/blob/master/src/view_fu... 9. https://github.com/paragonie/airship/blob/63cf0661ba21cbb3f3bfff510854a716f4b5579b/src/motifs.php#L18 https://github.com/paragonie/airship/blob/63cf0661ba21cbb3f3... 10. https://github.com/paragonie/airship/blob/2a8a87934921ecda85cf6ec3cecc43800b88a200/src/Cabin/Bridge/Controller/Account.php#L222 https://github.com/paragonie/airship/blob/2a8a87934921ecda85... 11. https://github.com/paragonie/airship/blob/eb4638eb31510028f40d6b51d13f0005fc5f2d60/src/Engine/Security/Util.php#L135 https://github.com/paragonie/airship/blob/eb4638eb31510028f4... 12. https://github.com/paragonie/airship/blob/eb4638eb31510028f40d6b51d13f0005fc5f2d60/src/Engine/Security/Util.php#L81 https://github.com/paragonie/airship/blob/eb4638eb31510028f4...

foota 10y ago
SQL injection is a solved problem, just use parameterized queries.
CiPHPerCoder 10y ago
Thank you for taking the time to look through the code. 1. Context matters: That's a CLI script used to build and sign core updates. We bundle it with the source code so anyone can fork our project and use only their own keys, easily. 2-4. This is a good point. https://github.com/paragonie/airship/issues/181 https://github.com/paragonie/airship/issues/181 > Properly escaping SQL injection is great (and something PHP apps seem to have a continual problem with), but why did you decide to roll your own framework for doing this? Because when Airship was started, everyone insisted on backwards compatibility for PHP 5 and I wanted to make use of strict typing. The master branch includes a static analyzer as part of continuous integration. > Are all of the well maintained, tested and great ones already available not suitable for some reason? Have you seen what I've done to PHP frameworks over the years? https://scott.arciszewski.me/open-source/ https://scott.arciszewski.me/open-source/ https://paragonie.com/security/advisories https://paragonie.com/security/advisories I wasn't in a hurry to pick up other peoples' technical debt, especially if we're paying money for vulnerabilities: https://hackerone.com/paragonie https://hackerone.com/paragonie 5. That's simply a paranoia/convenience feature. If you've got a better way to automagically convert filename.txt into filename-5.txt if filename.txt, filename-2.txt, filename-3.txt, and filename-4.txt already exist, I'm all ears. 6-10. Good eye. I think that can be refactored to repeat itself less. 11. I don't see a problem with this? 12. I don't see a problem with this either.
- orf 10y ago
  > Context matters: That's a CLI script. Ahh, my bad, it's still an odd way of doing this. And it's vulnerable to injection via the prompt parameter (not that it matters, but...). That pattern is repeated in a couple of places like the updater. > Because when Airship was started, everyone insisted on backwards compatibility for PHP 5 and I wanted to make use of strict typing. The master branch includes a static analyzer as part of continuous integration. Strict typing is great, but you don't have to write your own framework to benefit from it. Is PHP 5 not compatible with modern frameworks? > Have you seen what I've done to PHP frameworks over the years? > https://scott.arciszewski.me/open-source/ https://scott.arciszewski.me/open-source/ > https://paragonie.com/security/advisories https://paragonie.com/security/advisories > I wasn't in a hurry to pick up other peoples' technical debt. Indeed there are some vulnerabilities found in PHP libraries in those pages, but hardly enough to warrant your position. You aren't in a hurry to pick up others technical debt, but you rush ahead and create your own? All I'm saying is you put out a post saying you designed this from the ground up to be secure, but did you really? Is rolling your own ORM and homegrown framework secure? Says who? Yeah, you use modernish stuff like csp etc, but the code is still a typical PHP spaghetti of mixed concerns and hard-to-audit flows. It's not really a CMS, it's a NIH syndromed framework with a CMS on top of that. That's a typical source of bugs. > That's simply a paranoia/convenience feature. If you've got a better way to automagically convert filename.txt into filename-5.txt if filename.txt, filename-2.txt, filename-3.txt, and filename-4.txt already exist, I'm all ears. It could be done in a single query, I'm on my phone so i cant write you an example right now. > 11. I don't see a problem with this? > 12. I don't see a problem with this either. They are both framework functions, and you wouldn't be writing them if you had written this from the ground up to be secure IMO. It's kind of code smell. Using a whitelist is good though.

by orf 10y ago

foota 10y ago

SQL injection is a solved problem, just use parameterized queries.

CiPHPerCoder 10y ago

Thank you for taking the time to look through the code. 1. Context matters: That's a CLI script used to build and sign core updates. We bundle it with the source code so anyone can fork our project and use only their own keys, easily. 2-4. This is a good point. https://github.com/paragonie/airship/issues/181 https://github.com/paragonie/airship/issues/181 > Properly escaping SQL injection is great (and something PHP apps seem to have a continual problem with), but why did you decide to roll your own framework for doing this? Because when Airship was started, everyone insisted on backwards compatibility for PHP 5 and I wanted to make use of strict typing. The master branch includes a static analyzer as part of continuous integration. > Are all of the well maintained, tested and great ones already available not suitable for some reason? Have you seen what I've done to PHP frameworks over the years? https://scott.arciszewski.me/open-source/ https://scott.arciszewski.me/open-source/ https://paragonie.com/security/advisories https://paragonie.com/security/advisories I wasn't in a hurry to pick up other peoples' technical debt, especially if we're paying money for vulnerabilities: https://hackerone.com/paragonie https://hackerone.com/paragonie 5. That's simply a paranoia/convenience feature. If you've got a better way to automagically convert filename.txt into filename-5.txt if filename.txt, filename-2.txt, filename-3.txt, and filename-4.txt already exist, I'm all ears. 6-10. Good eye. I think that can be refactored to repeat itself less. 11. I don't see a problem with this? 12. I don't see a problem with this either.

orf 10y ago
> Context matters: That's a CLI script. Ahh, my bad, it's still an odd way of doing this. And it's vulnerable to injection via the prompt parameter (not that it matters, but...). That pattern is repeated in a couple of places like the updater. > Because when Airship was started, everyone insisted on backwards compatibility for PHP 5 and I wanted to make use of strict typing. The master branch includes a static analyzer as part of continuous integration. Strict typing is great, but you don't have to write your own framework to benefit from it. Is PHP 5 not compatible with modern frameworks? > Have you seen what I've done to PHP frameworks over the years? > https://scott.arciszewski.me/open-source/ https://scott.arciszewski.me/open-source/ > https://paragonie.com/security/advisories https://paragonie.com/security/advisories > I wasn't in a hurry to pick up other peoples' technical debt. Indeed there are some vulnerabilities found in PHP libraries in those pages, but hardly enough to warrant your position. You aren't in a hurry to pick up others technical debt, but you rush ahead and create your own? All I'm saying is you put out a post saying you designed this from the ground up to be secure, but did you really? Is rolling your own ORM and homegrown framework secure? Says who? Yeah, you use modernish stuff like csp etc, but the code is still a typical PHP spaghetti of mixed concerns and hard-to-audit flows. It's not really a CMS, it's a NIH syndromed framework with a CMS on top of that. That's a typical source of bugs. > That's simply a paranoia/convenience feature. If you've got a better way to automagically convert filename.txt into filename-5.txt if filename.txt, filename-2.txt, filename-3.txt, and filename-4.txt already exist, I'm all ears. It could be done in a single query, I'm on my phone so i cant write you an example right now. > 11. I don't see a problem with this? > 12. I don't see a problem with this either. They are both framework functions, and you wouldn't be writing them if you had written this from the ground up to be secure IMO. It's kind of code smell. Using a whitelist is good though.