-
Notifications
You must be signed in to change notification settings - Fork 560
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
[CVE-2018-18311] Integer overflow leading to buffer overflow #16560
Comments
From jmenon@isi.eduHi, As a part of an academic project, we have discovered an integer overflow in Perl which subsequently leads to a heap overflow. The vulnerability is present in Perl_my_setenv @ util.c : 2070 2070: void Perl_my_setenv(pTHX_ const char *nam, const char *val) { ... 2166: const int nlen = strlen(nam); ... 2171: vlen = strlen(val); 2172: new_env = (char*)safesysmalloc((nlen + vlen + 2) * sizeof(char)); Here, since the arguments nam and val are user controlled, the 32 bit integers nlen and vlen are also under the control of the attacker. Therefore, if nam and val are two very long strings, the addition at 2172 would result in an integer overflow. The new_env would therefore be a chunk of a size which is smaller than the sum of the lengths of the two input strings. This new_env is subsequently used in a call to memcpy to copy nlen bytes from nam and vlen bytes from val. This results in a buffer overflow on the heap with attacker controlled input. We have attached a perl script that demonstrates this vulnerability. Regards |
From jmenon@isi.edu |
From @iabynOn Tue, May 15, 2018 at 05:59:26PM -0700, Jayakrishna Menon wrote:
I don't think this has realistic security implications. I think it would The fix would appear to be to generally replace ints and I32s in -- |
The RT System itself - Status changed from 'new' to 'open' |
From jmenon@isi.eduJust to clarify, The attacker doesn't need to control both the key and value of a %ENV setting to trigger this bug. If the attacker can create a value of 0xFFFFFFFF bytes in length, this bug would be triggered. ________________________________ On Tue, May 15, 2018 at 05:59:26PM -0700, Jayakrishna Menon wrote:
I don't think this has realistic security implications. I think it would The fix would appear to be to generally replace ints and I32s in -- |
From jmenon@isi.eduIt has been more than 30 days since I have received any response regarding this issue. I had also made a pull request to patch this vulnerability[0]. However, that hasn't been merged even after 24 days. If there are some changes required in the patch I proposed in the pull request, please let me know and I will make the changes accordingly. [0] #13 Regards Jayakrishna Menon ________________________________ Just to clarify, The attacker doesn't need to control both the key and value of a %ENV setting to trigger this bug. If the attacker can create a value of 0xFFFFFFFF bytes in length, this bug would be triggered. ________________________________ On Tue, May 15, 2018 at 05:59:26PM -0700, Jayakrishna Menon wrote:
I don't think this has realistic security implications. I think it would The fix would appear to be to generally replace ints and I32s in -- |
From @iabynOn Thu, Jun 28, 2018 at 10:54:54PM +0000, Jayakrishna Menon wrote:
Note that the github repo is just a read-only mirror of the real perl
I've just pushed the following, which is similar to your commit, commit 34716e2 Perl_my_setenv(); handle integer wrap M util.c -- |
From @tonycozOn Fri, 29 Jun 2018 06:41:54 -0700, davem wrote:
+ sl = l * size; In the general case where size > 1 its possible for this multiply to overflow but still have sl > l. A safer check is something like: if (MEM_SIZE_MAX / size < l) Given that S_env_alloc() is only called with size != 1 when incrementally increasing the length of environ, this can't be practically attacked (environ would need to occupy a significant part of the address space), but might be a problem if the same code is re-used elsewhere or we have similar checks elsewhere. _MEM_WRAP_WILL_WRAP() does appear to do the right thing. None of the other code that calls croak_memory_wrap() does multiplication checks, so don't include this flaw. As to whether this is a security issue... I don't think it's possible to cause the overflow on a 32-bit system, since you'd need enough free address space to store a 2GB string, which is unlikely. It is (obviously) possible on a 64-bit system. If an attacker has control over the contents of the value that a perl program stores into a environment variable, and that perl program allows the value + name to be over 2GB, they can write beyond the end of the allocated buffer with attacker supplied data. This is a serious security issue. Writing beyond the end of an allocated buffer can be turned into code execution attacks, depending on the platform. Mitigations: Allowing an untrusted user to store ~2GB of data seems generally unsafe to me, and a program that uses that for example to supply data to a child process is going to run into system limits on common platforms much earlier[1][2]. Countering that, the program might be running on a platform without such limits, and have no need for portability. It also comes from someone[3] who grew up with fairly small memory systems, modern code should expect to work with larger amounts of data. If a perl user, mistaken or not decides to store a large amount of data in an environment variable we should either succeed or fail due to system limits, not write beyond the end of a buffer. So at this point I think this is a security issue[4]. Tony [1] https://blogs.msdn.microsoft.com/oldnewthing/20100203-00/?p=15083 |
From jmenon@isi.eduThank you for your attention. I am glad that you spared some time to investigate this issue. As you said, I also feel that this is indeed a serious security issue which is why I reported it. Hence, I would like to know if I could request a CVE-ID for this bug and if so, the procedures to obtain it. Regards Jayakrishna Menon ________________________________ On Fri, 29 Jun 2018 06:41:54 -0700, davem wrote:
+ sl = l * size; In the general case where size > 1 its possible for this multiply to overflow but still have sl > l. A safer check is something like: if (MEM_SIZE_MAX / size < l) Given that S_env_alloc() is only called with size != 1 when incrementally increasing the length of environ, this can't be practically attacked (environ would need to occupy a significant part of the address space), but might be a problem if the same code is re-used elsewhere or we have similar checks elsewhere. _MEM_WRAP_WILL_WRAP() does appear to do the right thing. None of the other code that calls croak_memory_wrap() does multiplication checks, so don't include this flaw. As to whether this is a security issue... I don't think it's possible to cause the overflow on a 32-bit system, since you'd need enough free address space to store a 2GB string, which is unlikely. It is (obviously) possible on a 64-bit system. If an attacker has control over the contents of the value that a perl program stores into a environment variable, and that perl program allows the value + name to be over 2GB, they can write beyond the end of the allocated buffer with attacker supplied data. This is a serious security issue. Writing beyond the end of an allocated buffer can be turned into code execution attacks, depending on the platform. Mitigations: Allowing an untrusted user to store ~2GB of data seems generally unsafe to me, and a program that uses that for example to supply data to a child process is going to run into system limits on common platforms much earlier[1][2]. Countering that, the program might be running on a platform without such limits, and have no need for portability. It also comes from someone[3] who grew up with fairly small memory systems, modern code should expect to work with larger amounts of data. If a perl user, mistaken or not decides to store a large amount of data in an environment variable we should either succeed or fail due to system limits, not write beyond the end of a buffer. So at this point I think this is a security issue[4]. Tony [1] https://blogs.msdn.microsoft.com/oldnewthing/20100203-00/?p=15083 |
From @tonycozOn Thu, 02 Aug 2018 20:02:48 -0700, jmenon@isi.edu wrote:
I'd like to wait a few more days to see if anyone else has an opinion. I'm not the only one whose opinion counts here. Sorry about the extended delay in handling this. Tony |
From @tonycozI plan to request a CVE ID for this issue in the next couple of days. If you've already requested an ID, please let me know. Thanks, |
From jmenon@isi.eduThank you for the update. And I have not requested a CVE ID for this issue. Regards Jayakrishna Menon ________________________________ I plan to request a CVE ID for this issue in the next couple of days. If you've already requested an ID, please let me know. Thanks, |
From jmenon@isi.eduThank you for the update I have not requested a CVE ID for this issue. Regards Jayakrishna Menon ________________________________ I plan to request a CVE ID for this issue in the next couple of days. If you've already requested an ID, please let me know. Thanks, |
From @tonycozOn Sun, 23 Sep 2018 23:40:01 -0700, tonyc wrote:
This is CVE-2018-18311. Tony |
From jmenon@isi.eduThank you so much for that. Regards Jayakrishna Menon ________________________________ On Sun, 23 Sep 2018 23:40:01 -0700, tonyc wrote:
This is CVE-2018-18311. Tony |
From @steve-m-hayMoved to public queue with the release of 5.26.3 and 5.28.1. |
From [Unknown Contact. See original ticket]Moved to public queue with the release of 5.26.3 and 5.28.1. |
From @karenetheridgeOn Thu, 29 Nov 2018 11:14:29 -0800, shay wrote:
Who has write access to the github repository? This pull request #13 should be closed. (Orthogonally, we should put some instructions up on github informing people that pull requests are not merged directly, but if they accompany their patch with a submission to the RT queue, the porters will be suitably informed of the patch's existence and can guide the submitter further.) |
From @steve-m-hayThis problem was resolved in blead by commit 34716e2, which was back-ported to 5.26.3 (5737d31) and 5.28.1 (0589f07). |
@steve-m-hay - Status changed from 'open' to 'resolved' |
From @steve-m-hayRe-opening as 'pending release': It's fixed and released in maint branches (5.26 and 5.28), but I guess it shouldn't be listed as 'resolved' until the blead fix is released in a stable version, which won't be until 5.30.0. |
@steve-m-hay - Status changed from 'resolved' to 'pending release' |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release today of Perl 5.30.0, this and 160 other issues have been Perl 5.30.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#133204 (status was 'resolved')
Searchable as RT133204$
The text was updated successfully, but these errors were encountered: