5 ms·

https://github.com/WayneD/rsync/commit/b7231c7d02cfb65d291af74ff66e7d8c507ee871#diff-a2ece3bcd75598be3f1c0a517c320f593557a5867f93382bcd2dfd850f5250b3 https://gi

https://github.com/WayneD/rsync/commit/b7231c7d02cfb65d291af74ff66e7d8c507ee871#diff-a2ece3bcd75598be3f1c0a517c320f593557a5867f93382bcd2dfd850f5250b3 https://github.com/WayneD/rsync/commit/b7231c7d02cfb65d291af... This newly added function, add_implied_include, which was committed as a part of the patch gives me the creeps. I do not claim to be competent enough to work with a utility as important as rsync, but looking at C code written like that makes me grateful for that fact.

testplzignore 4y ago
I'm glad I'm not the only one who thinks this. Clever parsing code, multiple commits to get it right (https://github.com/WayneD/rsync/commit/2f7c583143bc6e80902139c23d9d7283f88fbc6a https://github.com/WayneD/rsync/commit/2f7c583143bc6e8090213..., https://github.com/WayneD/rsync/commit/3d7015afa223494e3318495c2f5de9cb49229da9 https://github.com/WayneD/rsync/commit/3d7015afa223494e33184...), no tests, seemingly no code review. Not an attack on the maintainer of rsync - this pattern is typical for security patches. Within a few days, tools like Twistlock are going to alert on the CVE, and companies across the world will blindly upgrade without reviewing the change. Rinse and repeat thousands of times per year. Our industry continues to not take security seriously while pretending that it does.
- kbenson 4y ago
  My guess (hope) is that orgs like Red Hat will vet it a bit before putting it out, since they'll have to dive into the code and apply or reimplements it for multiple past versions of rsync. That actually covers a lot of companies using it. Paying (or at least relying on) another company to take care of that for you is what most places do. Honestly, that's better than tens of thousands of people wasting time on the same thing IMO.
- fsflover 4y ago
  If you care about security, you should not rely on a perfect code. Consider compartmentalization instead: https://qubes-os.org https://qubes-os.org. Works for me (or so I hope).
  - nonrandomstring 4y ago
    Also consider policy [1]. It's become a dirty word because it's been somewhat hijacked by management-class values and wrapped up in an unthinking "compliance and audits" mind-set. Thinking about what you want to allow, where to draw lines, planning in advance and sticking to it (including using tricks that force you to stick to it) is mature security thinking. You only have to be an organisation of one person to have a security policy. https://news.ycombinator.com/context?id=32248506 https://news.ycombinator.com/context?id=32248506
  - forgetfulness 4y ago
    That's of course on the side of due diligence on the user of any software, but most software should have tests, and certainly one of the importance of rsync, doubly so because it's written in C.
    - trasz 4y ago
      Tests aren’t particularly useful for detecting this kind of bugs though.
      - eikenberry 4y ago
        You add tests for this sort of thing after the fact to make sure the fix works as you think it does and to make sure the issue isn't re-introduced later. Tests can't find them, but they can help keep them away.
  - codedokode 4y ago
    I don't like the architecture because it looks like a set of ugly hacks. You don't need virtual machines to isolate applications because CPU already has a sandbox provided by protected mode. The problem is that legacy OS like Linux/Mac/Windows are unable to use this sandbox effectively. For example, here [1] they show a separate VM for the USB stack. But why do you need it if you can simply run USB stack in protected mode? Also, I assume that running so many VMs requires a lot of drive space and hurts performance. [1] https://www.qubes-os.org/intro/ https://www.qubes-os.org/intro/
- pjmlp 4y ago
  Our industry will only change when security exploits come with hard penalties for the companies that don't take security seriously, just like any food joint gets closed down when visited by the sanitary authorities and doesn't upheld the very minimum health conditions. Until then, rinse and repeat as you say.
  - netheril96 4y ago
    You can’t impose penalties for free and open source projects though. Well, you can, but that results in much fewer open source projects being ever developed.
    - pjmlp 4y ago
      Sure you can, just like people selling street food have to take care no one gets food poison.
      - account42 4y ago
        OTOH this also stops people from giving away food they don't need anymore. Something like good samaritan laws should apply here as well.
        pjmlp 4y ago
        That is why we have charities that take care of handling such issues.
- kraemahz 4y ago
  I am competent enough to read c and note that none of those changes you highlighted have functional impacts. They were largely cosmetic changes (e.g. removing an else condition by assigning the inclusive flags to a variable). I saw one change that looked like a performance improvement (an early loop exit break added). I will also note that rsync has a test suite and running tests as "integration" style testing is more common of shell utilities than writing gobs of unit tests. I would have liked to see an additional targeted test written for the CVE in this patch though.
deleted 4y ago
[deleted]
dboreham 4y ago
My review of that PR would be : "where's the test for this parsing code?". I'd expect to see many test inputs that are both valid and invalid. Note: I'm very much not a testing freak, but for this kind of code I think it's essential to have unit tests, if only because they provide necessary documentation about the intended functionality of the code.
- trasz 4y ago
  If you wrote a code that contains some bug, usually your your tests won’t catch it either. You’d need a fuzzer instead. But then again, it just doesn’t work this way - you can’t write perfect code, you need to follow the principle of least privilege, ie use compartmentalisation.
  - water8 4y ago
    What’s the point of compartmentalization, if all your airlocks are broken or unsecured?
    - trasz 4y ago
      Its not compartmentalization if your airlocks are broken or unsecured.
  - e12e 4y ago
    Well, using a fuzzer would probably be a good idea? See eg: > Fuzzing 100+ open source projects with OSS-Fuzz - lessons learned. (2021) https://adalogics.com/blog/fuzzing-100-open-source-projects-with-oss-fuzz https://adalogics.com/blog/fuzzing-100-open-source-projects-... Short hn discussion of post (but project has been featured many times): https://news.ycombinator.com/item?id=28391620 https://news.ycombinator.com/item?id=28391620
neoneye2 4y ago
What if the input contains multiple `/./` ? It seems that `cp = strstr(arg, "/./");` only checks once. Maybe wrap in a loop?
- account42 4y ago
  No, this is for handling the --relative (-R) flag where the part after the first /./ in each source path is a relative destination for that path so e.g. rsync -aR foo/./bar/baz/ dest/ copies data from foo/bar/baz/ to dest/bar/baz/ while any other /./ is effectively meaningless.
js2 4y ago
This repo's commit messages are not good: "A few more minor tweaks." "A few more minor changes." "Some extra file-list safety checks." https://github.com/WayneD/rsync/commits/master https://github.com/WayneD/rsync/commits/master Those are the full commit messages. By way of comparison, here's what commit messages to git itself look like: https://github.com/git/git/commit/198551ca54f6ff1c95c9357ccca9ae37298811bf https://github.com/git/git/commit/198551ca54f6ff1c95c9357ccc... https://github.com/git/git/commit/dee8a1455c8ad443ef59e0d5b7c168886e50b9ea https://github.com/git/git/commit/dee8a1455c8ad443ef59e0d5b7... Each commit contains paragraphs of explanatory material. Jeff King's commit messages are "gold standards" to me, often well exceeding the size of the diff itself: https://github.com/git/git/commit/aa0834a04e1d9d3ab81ecd4a4a138233e6e234ed https://github.com/git/git/commit/aa0834a04e1d9d3ab81ecd4a4a... https://github.com/git/git/commit/38f865c27d1f2560afb48efd2b7b105c1278c4b5 https://github.com/git/git/commit/38f865c27d1f2560afb48efd2b... https://github.com/git/git/search?q=author%3Apeff&type=commits https://github.com/git/git/search?q=author%3Apeff&type=commi... Please folks, I beg of you, spend time writing your commit messages. You're not writing them for you today. You're writing them for others, including your future self. They should explain the commit: What is its goal? Why does it exist? Why was it written in this way? What other context does the developer need to understand this commit? A PR description is not a substitute for a good commit message for multiple reasons: 1. If the PR is a single commit, then it's just the commit message and your job is probably done. 2. If there are multiple commits, then the PR description should summarize the goal of the aggregate of its commits. 3. The PR description is typically written hours or days after the commit(s). What was fresh in your head when you wrote that code is now stale and you will struggle to recall why you made a change in a particular way more than if you wrote it down fresh when you commit the change. 4. The PR description is not part of the repo's history. It requires access to a (typically propriety) platform to read. So please, https://cbea.ms/git-commit/ https://cbea.ms/git-commit/
- bombcar 4y ago
  And if you work by making many rapid small commits, rebase and squash when you're "done" and then add the large, longer commit message to the "new big commit".
- tweetle_beetle 4y ago
zelphirkalt 4y ago
So many if, else, case, while, whatever nested control flow, all in one procedure, that one can almost be sure, that one of those branches of execution will have a bug somewhere. Very short variable names, sometimes 1 character or 2 and pointer magic. Looks like a prime example of badly readable code.

testplzignore 4y ago

I'm glad I'm not the only one who thinks this. Clever parsing code, multiple commits to get it right (https://github.com/WayneD/rsync/commit/2f7c583143bc6e80902139c23d9d7283f88fbc6a https://github.com/WayneD/rsync/commit/2f7c583143bc6e8090213..., https://github.com/WayneD/rsync/commit/3d7015afa223494e3318495c2f5de9cb49229da9 https://github.com/WayneD/rsync/commit/3d7015afa223494e33184...), no tests, seemingly no code review. Not an attack on the maintainer of rsync - this pattern is typical for security patches. Within a few days, tools like Twistlock are going to alert on the CVE, and companies across the world will blindly upgrade without reviewing the change. Rinse and repeat thousands of times per year. Our industry continues to not take security seriously while pretending that it does.

kbenson 4y ago
My guess (hope) is that orgs like Red Hat will vet it a bit before putting it out, since they'll have to dive into the code and apply or reimplements it for multiple past versions of rsync. That actually covers a lot of companies using it. Paying (or at least relying on) another company to take care of that for you is what most places do. Honestly, that's better than tens of thousands of people wasting time on the same thing IMO.
fsflover 4y ago
If you care about security, you should not rely on a perfect code. Consider compartmentalization instead: https://qubes-os.org https://qubes-os.org. Works for me (or so I hope).
- nonrandomstring 4y ago
  Also consider policy [1]. It's become a dirty word because it's been somewhat hijacked by management-class values and wrapped up in an unthinking "compliance and audits" mind-set. Thinking about what you want to allow, where to draw lines, planning in advance and sticking to it (including using tricks that force you to stick to it) is mature security thinking. You only have to be an organisation of one person to have a security policy. https://news.ycombinator.com/context?id=32248506 https://news.ycombinator.com/context?id=32248506
- forgetfulness 4y ago
  That's of course on the side of due diligence on the user of any software, but most software should have tests, and certainly one of the importance of rsync, doubly so because it's written in C.
  - trasz 4y ago
    Tests aren’t particularly useful for detecting this kind of bugs though.
    - eikenberry 4y ago
      You add tests for this sort of thing after the fact to make sure the fix works as you think it does and to make sure the issue isn't re-introduced later. Tests can't find them, but they can help keep them away.
- codedokode 4y ago
  I don't like the architecture because it looks like a set of ugly hacks. You don't need virtual machines to isolate applications because CPU already has a sandbox provided by protected mode. The problem is that legacy OS like Linux/Mac/Windows are unable to use this sandbox effectively. For example, here [1] they show a separate VM for the USB stack. But why do you need it if you can simply run USB stack in protected mode? Also, I assume that running so many VMs requires a lot of drive space and hurts performance. [1] https://www.qubes-os.org/intro/ https://www.qubes-os.org/intro/
pjmlp 4y ago
Our industry will only change when security exploits come with hard penalties for the companies that don't take security seriously, just like any food joint gets closed down when visited by the sanitary authorities and doesn't upheld the very minimum health conditions. Until then, rinse and repeat as you say.
- netheril96 4y ago
  You can’t impose penalties for free and open source projects though. Well, you can, but that results in much fewer open source projects being ever developed.
  - pjmlp 4y ago
    Sure you can, just like people selling street food have to take care no one gets food poison.
    - account42 4y ago
      OTOH this also stops people from giving away food they don't need anymore. Something like good samaritan laws should apply here as well.
      - pjmlp 4y ago
        That is why we have charities that take care of handling such issues.
kraemahz 4y ago
I am competent enough to read c and note that none of those changes you highlighted have functional impacts. They were largely cosmetic changes (e.g. removing an else condition by assigning the inclusive flags to a variable). I saw one change that looked like a performance improvement (an early loop exit break added). I will also note that rsync has a test suite and running tests as "integration" style testing is more common of shell utilities than writing gobs of unit tests. I would have liked to see an additional targeted test written for the CVE in this patch though.

deleted 4y ago

[deleted]

dboreham 4y ago

My review of that PR would be : "where's the test for this parsing code?". I'd expect to see many test inputs that are both valid and invalid. Note: I'm very much not a testing freak, but for this kind of code I think it's essential to have unit tests, if only because they provide necessary documentation about the intended functionality of the code.

trasz 4y ago
If you wrote a code that contains some bug, usually your your tests won’t catch it either. You’d need a fuzzer instead. But then again, it just doesn’t work this way - you can’t write perfect code, you need to follow the principle of least privilege, ie use compartmentalisation.
- water8 4y ago
  What’s the point of compartmentalization, if all your airlocks are broken or unsecured?
  - trasz 4y ago
    Its not compartmentalization if your airlocks are broken or unsecured.
- e12e 4y ago
  Well, using a fuzzer would probably be a good idea? See eg: > Fuzzing 100+ open source projects with OSS-Fuzz - lessons learned. (2021) https://adalogics.com/blog/fuzzing-100-open-source-projects-with-oss-fuzz https://adalogics.com/blog/fuzzing-100-open-source-projects-... Short hn discussion of post (but project has been featured many times): https://news.ycombinator.com/item?id=28391620 https://news.ycombinator.com/item?id=28391620

neoneye2 4y ago

What if the input contains multiple `/./` ? It seems that `cp = strstr(arg, "/./");` only checks once. Maybe wrap in a loop?

account42 4y ago
No, this is for handling the --relative (-R) flag where the part after the first /./ in each source path is a relative destination for that path so e.g. rsync -aR foo/./bar/baz/ dest/ copies data from foo/bar/baz/ to dest/bar/baz/ while any other /./ is effectively meaningless.

js2 4y ago

This repo's commit messages are not good: "A few more minor tweaks." "A few more minor changes." "Some extra file-list safety checks." https://github.com/WayneD/rsync/commits/master https://github.com/WayneD/rsync/commits/master Those are the full commit messages. By way of comparison, here's what commit messages to git itself look like: https://github.com/git/git/commit/198551ca54f6ff1c95c9357ccca9ae37298811bf https://github.com/git/git/commit/198551ca54f6ff1c95c9357ccc... https://github.com/git/git/commit/dee8a1455c8ad443ef59e0d5b7c168886e50b9ea https://github.com/git/git/commit/dee8a1455c8ad443ef59e0d5b7... Each commit contains paragraphs of explanatory material. Jeff King's commit messages are "gold standards" to me, often well exceeding the size of the diff itself: https://github.com/git/git/commit/aa0834a04e1d9d3ab81ecd4a4a138233e6e234ed https://github.com/git/git/commit/aa0834a04e1d9d3ab81ecd4a4a... https://github.com/git/git/commit/38f865c27d1f2560afb48efd2b7b105c1278c4b5 https://github.com/git/git/commit/38f865c27d1f2560afb48efd2b... https://github.com/git/git/search?q=author%3Apeff&type=commits https://github.com/git/git/search?q=author%3Apeff&type=commi... Please folks, I beg of you, spend time writing your commit messages. You're not writing them for you today. You're writing them for others, including your future self. They should explain the commit: What is its goal? Why does it exist? Why was it written in this way? What other context does the developer need to understand this commit? A PR description is not a substitute for a good commit message for multiple reasons: 1. If the PR is a single commit, then it's just the commit message and your job is probably done. 2. If there are multiple commits, then the PR description should summarize the goal of the aggregate of its commits. 3. The PR description is typically written hours or days after the commit(s). What was fresh in your head when you wrote that code is now stale and you will struggle to recall why you made a change in a particular way more than if you wrote it down fresh when you commit the change. 4. The PR description is not part of the repo's history. It requires access to a (typically propriety) platform to read. So please, https://cbea.ms/git-commit/ https://cbea.ms/git-commit/

bombcar 4y ago
And if you work by making many rapid small commits, rebase and squash when you're "done" and then add the large, longer commit message to the "new big commit".
tweetle_beetle 4y ago

zelphirkalt 4y ago

So many if, else, case, while, whatever nested control flow, all in one procedure, that one can almost be sure, that one of those branches of execution will have a bug somewhere. Very short variable names, sometimes 1 character or 2 and pointer magic. Looks like a prime example of badly readable code.