premake / premake-core

Premake
https://premake.github.io/
BSD 3-Clause "New" or "Revised" License
3.23k stars 621 forks source link

Need confirmation for some unpatched CVE in Curl #2156

Open the-Chain-Warden-thresh opened 1 year ago

the-Chain-Warden-thresh commented 1 year ago

I'm cloning this repo to make some modifications to customize. However, I've noticed that some CVEs which were confirmed and fixed by curl do not get patched in this repo. To enhance the availability of my project as far as possible, I will appreciate it if any of the CVE below do exist in this repo as well, so that I can fix these security issue myself by applying the corresponding patch. Here are the CVEs I found in this repo unpatched, but get fixed in curl:

CVE-2018-1000121 in contrib/curl/lib/openldap.c's function static ssize_t ldap_recv(struct connectdata *conn, int sockindex, char *buf, size_t len, CURLcode *err), with patch here for your reference.

CVE-2018-16890 in contrib/curl/lib/vauth/ntlm.c's function static CURLcode ntlm_decode_type2_target(struct Curl_easy *data, unsigned char *buffer, size_t size, struct ntlmdata *ntlm), with patch here for your reference.

CVE-2018-1000122 in contrib/curl/lib/transfer.c's function static CURLcode readwrite_data(struct Curl_easy *data, struct connectdata *conn, struct SingleRequest *k, int *didwhat, bool *done, bool *comeback), with patch here for your reference.

nickclark2016 commented 1 year ago

Instead of manually applying patches, we'd rather just update our build of curl.

swagtoy commented 1 year ago

I've complained about this before somewhere in #2057 ... I've discussed about ways to fetch an up-to-date curl build, and i also didn't mention pulling curl from source.

What should be happening is that Curl gets pulled and either patches get applied (per OS?) or defer to OS libraries (on linux, bsd, or even windows now that Curl seems bundled in). Curl is designed to build on quite literally everything and the reasoning for maintaining custom patches seems strange to me.

I even had to manually patch some pieces on openbsd iirc, which obviously goes to show that this isn't really a portable solution anyway. It should at least fall back to pkg-config or something. Many CMake projects even do that

I want to tag @bagder, I think i asked his input in IRC a long long time ago on bundling old Curl in this project. Maybe he will have better resourcing of what's going on here.

the-Chain-Warden-thresh commented 1 year ago

Instead of manually applying patches, we'd rather just update our build of curl.

That is to say, all of the 3 CVE I've listed previously do impact this repo? Thanks a lot for your reply, then I'll apply the corresponding patch in order to start my own work as soon as possible. After you update the curl code, I'll keep up with your newer version to continue my customization.

the-Chain-Warden-thresh commented 1 year ago

I've complained about this before somewhere in #2057 ... I've discussed about ways to fetch an up-to-date curl build, and i also didn't mention pulling curl from source.

What should be happening is that Curl gets pulled and either patches get applied (per OS?) or defer to OS libraries (on linux, bsd, or even windows now that Curl seems bundled in). Curl is designed to build on quite literally everything and the reasoning for maintaining custom patches seems strange to me.

I even had to manually patch some pieces on openbsd iirc, which obviously goes to show that this isn't really a portable solution anyway. It should at least fall back to pkg-config or something. Many CMake projects even do that

I want to tag @bagder, I think i asked his input in IRC a long long time ago on bundling old Curl in this project. Maybe he will have better resourcing of what's going on here.

A better package management mechanism can definitely minimize similar issues like this as much as possible, but I'd rather manually apply patches so that I can start my own work as soon as possible.

bagder commented 1 year ago

I don't have much to contribute when it comes how you should handle curl in your project. I'm convinced you know much more about that and your project's specifics than I do.

In curl, we document every past curl CVE with lots of details here: https://curl.se/docs/security.html

swagtoy commented 1 year ago

I don't have much to contribute when it comes how you should handle curl in your project. I'm convinced you know much more about that and your project's specifics than I do.

@bagder https://premake.github.io/docs/http.download - Seems like a fairly common endpoint.

Generally speaking, it's absolutely possible for someone to hijack their server if they offer downloads to something, no? At least with such an old version of libcurl, do you know any sort of specific remote-execution since (2018) then, or any that are particularly easy to pull off? I probably, and maybe others, aren't worried about DoS attacks or assert-like crashes. The real issue can be remote attacks.

Most importantly, Look at these very small commits / patches (peep the local curl changes) to the actual Curl distribution, do they come off a bit strange or hacky?

A better package management mechanism can definitely minimize similar issues like this as much as possible, but I'd rather manually apply patches so that I can start my own work as soon as possible.

@the-Chain-Warden-thresh Generally speaking, everything local should be done in a Patch file (or several patch files), which gets applied to the latest stable release of Curl. Instead, we're just bloating up a Git repository with a copy of Curl (and other repositories). It just seems quite wrong.

nickclark2016 commented 1 year ago

Personally, I'd prefer to either be on the latest version of libcurl (and automatically update somehow) or use the OS built-in libraries. That said, I do not have the time to investigate and implement either of these approaches. If there are security vulnerabilities and patching would be an immediate stopgap, I'd happily review patches to merge them in. I'd also happily other changes that more security minded people (like yourselves) could contribute from libcurl to the repo. I can honestly say that security is not in my core skillset, so I'll be happy to review any changes or recommendations made.

swagtoy commented 1 year ago

That would probably be the best approach entirely, and honestly I'd really love to submit a PR, specifically for each library, but I would also like a greenlight before I even put in the effort (i've written code here before and got rejected). Specifically, I would delete all the old libraries' code (bare in mind, most repos like this use Git submodules) although the git clone damage may be done, but again, I could arguably: fetch a fixed-stable release (or maybe if the source code has an option for latest stable release), apply patches which we should avoid as much as possible, rinse and repeat. Of course, all that is done before I run a pkg-config or search for locally-installed libraries, as some can have custom patches to fix issues specific to OS'.

nickclark2016 commented 1 year ago

@starkos @samsinsane Please take a look at this. I give it my approval, but I'd prefer to have one of you also on the thread for a greenlight.

samsinsane commented 1 year ago

There's a lot of things to respond to here.

Updating dependencies

Yeah, sounds good. However, not all users will be happy with this happening. For example, when we moved from Lua 5.1 to 5.3, this broke compatibility with LuaJIT which caused issues for some users. So, not everyone will be interested in updating/security at all costs.

Using system libraries

This is at odds with another issue (#632). Since binaries are made available on the GitHub releases, it makes more sense to continue to bundle the dependencies as we currently do. Providing an option for users to build with system libraries is fine, but it shouldn't come at the detriment of the existing workflow.

Downloading the latest versions of dependencies

Personally, not interested in this. The current set of prerequisites for building Premake is Visual Studio or make+GCC (or drop-in replacements). Personally, this lower barrier to entry was one of the reasons I got into working on Premake in the first place. I didn't need to setup a bunch of random dependencies just to build Premake, or to extend it. This made it easier to just get on with what I wanted to do, which lead to me becoming a maintainer of the project.

Using submodules instead of copying code

I'm in favour of this, with the caveat of the above - it shouldn't increase the prerequisites for building Premake.


Having said all of that, I think it's important to have @starkos provide his input before changing how the project is structured. I know he hasn't had time to respond to issues recently but I'm not a fan of changing the project while he's unable to approve or provide input.

Crispy-fried-chicken commented 9 months ago

@samsinsane Any further confirmation for using the submodule in premake? I've found some other CVEs in the current curl used in premake now like CVE-2020-8231 and CVE-2017-7468. Besides, I also found that the lua used in this repo contains a CVE-2020-24370, maybe update that to a newer version together?

samsinsane commented 9 months ago

No, there's been no progress on this. Any progress will be noted in this issue.