fix for integer overflow.

[ihsinme]

my tests have shown a practical possibility of overflow implementation, so I consider it necessary to make the code more secure.
this check will eliminate the possibility of linelen integer overflow, followed by a controlled heap overflow.

[bagder]

Good catch! When I reviewed your fix, it also dawned on me how silly this function is. It allows a config files without any line length limits and it uses at least one alloc per line it reads - in addition to the integer overflow risk you've fixed. Very ineffective.

I would thus instead suggest we refactor this function into not reallocing, using a fixed length line limit and just alloc the buffer once for the entire file. I submitted #5941 trying that. It also has the benefit that it is less code in the final result.

[ihsinme]

your fix really looks more professional and in-depth.

however, I would be extremely grateful if you would allow me to have a good PR, perhaps before accepting yours. this would allow me to continue to develop in the direction of information security.

thank.

[bagder]

I would be extremely grateful if you would allow me to have a good PR, perhaps before accepting yours

I will of course appreciate and accept all PRs you provide that improve curl! Our job here is to make curl as good as possible.

[ihsinme]

thank you.
if I can do anything else within this PR, let me know.

[bagder]

Thanks, I prefer to go with the #5941 approach, as it fixes the overflow issue and is a general speed improvement for the loop at the same time.

[ihsinme]

it is a pity that you did not accept my PR at least temporarily

[bagder]

what would be the point in that?

[ihsinme]

thanks for the question, maybe the problem is in my english.
I wrote above that the adoption of such a pr is not a problem for you, but for me it is an assessment of my work and time spent. I need this to develop in the direction of searching for security errors.
if it is still possible I would be grateful for accepting this PR. realizing that in the near future you will still accept yours.

[bagder]

That's not how things work in the curl project, and in fact usually not in any open source projects I've been involved with.

We merge code that improves the project, the code. We do not merge code just to make the author of the patch happy. Hopefully of course we do both.

When we have a problem to fix, and we have more than one way to fix it, we discuss and land the best fix we can come up with. It would be crazy and pointless to land "intermediary" fixes that will be overwritten just a day later. That will not happen on my watch. There's absolutely no benefit for the project in doing do.

[ihsinme]

it's great how you describe the process, but I still think there are many buts.

1. we are talking about security issues and not fixes in general.
   2.we are talking about my role, in this case I created 5940 you said super and created 5941, rejecting 5940. (thus it is not clear if my work was useful at all, it is difficult to track down later) (even if you continued to improve 5940 and that would be better)
2. regarding the search for always the most general solution, this is also controversial, for example 5912 you made privately, and did not bother with creating a generalizing PR.

I ask you to understand me correctly, although it must be difficult.
you are trying to create a nice beautiful project, and curl is really good at it, you have beautiful safe code.
but I have a different role, I am looking for problems in the code of different projects, I perfectly understand that I will not be as good at coding as you are, but if I do not have the opportunity to develop the search for errors interacting with you, I will have to accumulate the found errors to build more serious exposure and sell this information.
at this stage, perhaps I am mistaken due to inexperience, but the way of interaction with the manufacturer at the earliest stage is the most ethical and should be developed.

ps it is worth noting that some projects operate as I describe, i.e. if the error is valid, they accept my fix and then quietly work on improving their code. for example (PowerDNS/pdns#9320)

[bagder]

we are talking about security issues and not fixes in general

You have decided that your fixes need a different path into curl than other changes do. I disagree. They've been bug-fixes and I think our process for handling such works pretty well. Discussing and improving fixes before we land them is a natural step in development.

we are talking about my role

This is the real issue. You're very focused on you here and that I should merge your PRs no matter what, even if I don't even like them. That doesn't help curl and frankly it doesn't help you become a better committer either.

it is not clear if my work was useful at all

Sure it is. We give credits to everyone that helps out (barring mistakes of course). The PR we already merge has your "name" in it (name within quotes since I don't actually know your name just your github user name), and in the PR I created the other day it says "reported-by". Giving proper credit. How else are you saying we should make it clear?

I think your primary mistake is to assume that merging PRs is the only way for us to say thanks and give credit to the reporters, the contributors and the helpers. There are many ways to contribute and to help, writing the actual patch is one way but it doesn't rule out that we appreciate the other people involved. And we say thanks and give credit to those as well.

for example 5912 you made privately

I don't understand. #5912 was your PR, I didn't do anything "privately" for that? I merged it, you're the author.

it is worth noting that some projects operate as I describe, i.e. if the error is valid, they accept my fix

That's their prerogative. I think that's the completely wrong way to do software engineering. Looking at their handing of your PR there, it's not even clear to me that your description actually matches reality.

[jzakrzewski]

@ihsinme Your work is appreciated. You have discovered the issue and it'll be fixed. The fact that with @bagder's PR and not yours is irrelevant and does not diminish your role in discovering the problem.

We all want our PRs to be merged but sometimes there's simply a better approach to fix something and it doesn't make sense to merge something for the sake of merging it.

As a side note - security researchers do not have to fix found issues. They can but this is not their primary role.

[ihsinme]

Of course, it is unpleasant and offensive when they tell you that you are too fixated on yourself or say that your other pr is not understandable at all, for the fact that you are just looking for errors in the code and want your work to be visible, it seems to me unprofessional.

I would like you to understand how the world looks from my eyes, it seems to me that if you understand, you can find a compramis that will give a win for the project, and so on.

1. the Curl project, probably as a team, is very professional in my opinion. I say this on the basis of analysis by fuzzers and static analyzers. it's really cool. it is very difficult to find an error in your case not during operation, but by searching.
2. if you find an error and it is actually confirmed in practice, then you need to get profit from the search further, if this is not the case, then you will not be able to grow professionally.
3. The first thing that comes to mind is an exploit and an attempt at bugbaunty. but here is the complexity, even if I create a poc and describe the attack scenario, then you say this is not included in our threat model because ...root, local.
4. I decide to prepare a fix and at least get a profit. In order to appreciate your PR practice, I am posting a simple 5912. it is accepted. no one tells me that it is not professional, that it should be generalized to the entire code, that you will make another fix.
5. Based on this I am preparing a simple error correction, I emphasize that my correction is not good or bad, it is just simple. I am told ok super, but we will do something else, I draw your attention to the fact that it is important for me that my PR is accepted. and they tell me attention well we will accept (as far as I understand the answer)

"I would be extremely grateful if you would allow me to have a good PR, perhaps before accepting yours

I will of course appreciate and accept all PRs you provide that improve curl! Our job here is to make curl as good as possible. "

6. then everything goes the other way around and all my questions about why. they answer me why, it makes no sense, it is not necessary, it is not professional.

Believe me, I understand that you are professionals, but I also understand that opensource to become better not only thanks to the efforts of people like you, but also like me. and to be honest, after this experience that I got communicating with you, I am at least not pleased.

so my message is simple, fixing security bugs is quite a narrow area and it is necessary to create more favorable conditions for those who spend their time finding these bugs. (even in the current conditions, nothing would change for you, but it would have a colossal positive impulse for me if you did not generate a new PR, but continued to improve mine.)

in any case, I am grateful for the opportunity to discuss this question and for the answers, this is important to me.

[bagder]

I'm sorry we made you doubt that your help and fixes are appreciated. We very much appreciate getting your help to improve curl. Appreciation doesn't imply merging your PR as is though.

it would have a colossal positive impulse for me if you did not generate a new PR, but continued to improve mine

If it had been an improvement or iteration of what you did, I would have. I instead took a completely different approach and then it wasn't at all like your PR did it anymore. I used your report about the problem as input and reworked it into a fix I think was better and more over all covering.

You know I've spent way more time answering your complaints and whining than it took me to write that PR, and as you might've seen I've also in the mean time written a second PR that is yet another take on the problem. Another PR that is far away from the change you did and therefore not based on your patch. It is still based on your initial report and you will get credited for it, as we try to do with all bug reports.

Again: this is how we work in the project. It is nothing personal about you or your fix. We just want the best possible fix to get landed and when we see improvements and manage to produce them, then we consider those instead of the initially suggested one. There's just no logical or sensible reason for you or anyone to object to that. Why would someone who claim to look and care for security problems in curl argue that we would land an inferior fix when already have a better one?

Can we stop this now and go back to improve curl?

[jay]

I will have to accumulate the found errors to build more serious exposure and sell this information.

so my message is simple, fixing security bugs is quite a narrow area and it is necessary to create more favorable conditions for those who spend their time finding these bugs.

We have a bug bounty and you should file security reports at hackerone, not here. I'd guess a report like this would rate fairly low. We give attribution for all bugs, security and otherwise by using 'Reported-by:` in the commit message of the fix. Reporting an issue does not entitle you to author the fix, but if you were to write one then it is considered. In this instance we have a bigger issue of design and are still discussing the best way to handle allocations like these.

See also:
curl - Security process

[ihsinme]

thanks for answers