Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SECURITY.md #83

Open
zidingz opened this issue Nov 9, 2021 · 9 comments
Open

Add SECURITY.md #83

zidingz opened this issue Nov 9, 2021 · 9 comments

Comments

@zidingz
Copy link

zidingz commented Nov 9, 2021

Hey there!

I belong to an open source security research community, and a member (@geeknik) has found an issue, but doesn’t know the best way to disclose it.

If not a hassle, might you kindly add a SECURITY.md file with an email, or another contact method? GitHub recommends this best practice to ensure security issues are responsibly disclosed, and it would serve as a simple instruction for security researchers in the future.

Thank you for your consideration, and I look forward to hearing from you!

(cc @huntr-helper)

@ssloy
Copy link
Owner

ssloy commented Nov 9, 2021

Seriously, a security issue in a series of lectures on rendering?

@ssloy ssloy closed this as completed Nov 9, 2021
@geeknik
Copy link

geeknik commented Nov 9, 2021

Yes. There are multiple integer overflows present in the code you’re sharing as part of this repository. 🤙🏻

@Joeppie
Copy link

Joeppie commented Nov 9, 2021

Yes. There are multiple integer overflows present in the code you’re sharing as part of this repository. 🤙🏻

Doesn't really seem to constitute any kind of serious security issue (is it an attack vector of any kind?) . But to address @ssloy 's valid question; I think coding (security) issues ought to be taken seriously. People use code in real life scenarios, even from lectures. It of course make no sense to consider any issue in this code as directly compromising any system.

@geeknik perhaps have the member simply report the issue via a bug report, if there is confidence that no existing system is directly compromised by the vulnerability in question? If the existence of a type of bug is not yet publicly known (zero-day), then probably informing a low-risk project about is counterproductive, even if done 'responsibly'.

@geeknik
Copy link

geeknik commented Nov 9, 2021

Good to know being responsible is counter productive. I’ll keep the information to myself and move on to other projects which are more receptive to receiving information about their published code. Have a great day!

@Joeppie
Copy link

Joeppie commented Nov 9, 2021

Good to know being responsible is counter productive. I’ll keep the information to myself and move on to other projects which are more receptive to receiving information about their published code. Have a great day!

Firstly, I am not a contributor myself so I apologize to @ssloy for even taking any part in any discussion of this, secondly; I am not saying that "being responsible is counter-productive".

The lack of professional attitude and the tangential nature of the supposed 'security problem' lead me to question the validity of your supposed and undocumented findings. Unsollicited as it may be of me to do so, I view your communication in this project as an example of virtue signalling

Virtue signalling is a pejorative neologism for the expression of a disingenuous moral viewpoint with the intent of communicating good character

@geeknik
Copy link

geeknik commented Nov 9, 2021

Since @Joeppie has decided to violate GitHub policies by personally attacking me, I have blocked and reported them. I was trying to be responsible and disclose these issues in a private setting, but I guess that isn't how things are supposed to be done, so here is the public disclosure that @Joeppie insisted upon during his name calling.

The multiplication results may overflow 'int' before it is converted to 'size_t' in the following locations of tgaimage.cpp. This is indicative of an integer overflow or wraparound situation:

Line 32: size_t nbytes = bytespp*width*height;
Line 62: size_t pixelcount = width*height;
Line 134: out.write(reinterpret_cast<const char *>(data.data()), width*height*bytespp);
Line 172: size_t npixels = width*height; Line 247: size_t bytes_per_line = width*bytespp;
Line 264: data = std::vector<std::uint8_t>(width*height*bytespp, 0);
Line 269: std::vector<std::uint8_t> tdata(w*h*bytespp, 0);
Line 273: size_t nlinebytes = w*bytespp;
Line 274: size_t olinebytes = width*bytespp;

We were going to recommend using a cast to ensure that the multiplication is done using the larger integer type to avoid overflow, but I'm sure @Joeppie is better suited to recommend how to proceed from here. 👍🏻

@ssloy
Copy link
Owner

ssloy commented Nov 9, 2021

@geeknik Security.md is an overkill; writing a direct mail is way less efficient for my repositories than opening an issue. If you find an UB, open an issue, and I'll gladly look into it.

Thank you for sharing your findings, I am reopening the issue.

That is being said, I am really surprised by your aggressive behaviour.

@ssloy ssloy reopened this Nov 9, 2021
@Joeppie
Copy link

Joeppie commented Nov 9, 2021

Since @Joeppie has decided to violate GitHub policies by personally attacking me, I have blocked and reported them. I was trying to be responsible and disclose these issues in a private setting, but I guess that isn't how things are supposed to be done, so here is the public disclosure that @Joeppie insisted upon during his name calling.

@geeknik I don't see any personal attack, my feedback has been to highlight behavior that I consider to be negative and counter-productive. In the same negative comment preceding your last, you also entirely dismissed my suggestion to simply report the issue, which you have now done after all.

Remember, it is you who gravely escalated the issue by effectively insuinating that:

  1. I (Joeppie) feel you are counter productive
  2. this entire project does not take security to heart (this forced me to respond, to clear the air on the apparent misunderstanding; I have not contributed to this project)
  3. That I don't take security to heart (this is entirely untrue, I took an obvious interest)
  4. That I dismissed the issue (I did not, I questioned it, which any rational person should do)
  5. This project is less good security-wise than other projects (which I find quite a stab, I follow this project because I appreciate the work done by its actual contributors.)
  6. You will not share the information anymore as a result of 'negative' or 'dismissive' feedback. (even though I acknowledge your question)

You did so in but a few short words;

Good to know being responsible is counter productive. I’ll keep the information to myself and move on to other projects which are more receptive to receiving information about their published code. Have a great day!

None of these things are true; In my communication I may have been direct, but I have also been transparent and respectful. In no way have I attacked you as a person, what I have done is questioned and criticized your behavior and chosen approach, which I do find to be counter-productive. I personally feel no need to report your latest comment, even though you chose to extrapolate on my apparently negative thoughts and actions. (Weird, since from the get-go I expressed that I concur with your actual opinion that it should be dealt with.) Here is a quote directly from the original comment you chose to take offense to:

. But to address @ssloy 's valid question; I think coding (security) issues ought to be taken seriously. People use code in real life scenarios, even from lectures.

The question, above I refered to was

Seriously, a security issue in a series of lectures on rendering?

@geeknik I also feel the need to point out that although I am not reporting this (the moderators undoubtedly have better things to do) I feel that I am being bullied by this. If you reported to Github, why explicitly threaten this here? It is not relevant to the discussion. This makes me feel genuinely unsafe and unwelcome to contribute (which is why there even are community guidelines in the first place)

I look forward to any response or reaction from Github with confidence and hope no further misunderstandings occur.

@mikkorantalainen
Copy link

This is indicative of an integer overflow or wraparound situation:

Do you have an example how attacker could cause overflow for any of those multiplications? Did you generate that list automatically simply by taking all integer multiplications or was there some additional human screening process for this list?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants