Possible string overrun with invalid len in gv.c #15598
Comments
From @toddrCreated by @toddrWe ran across this issue during B::C development. It turns out that if for (nend = name; *nend || nend != (origname + len); nend++) { I did not search for other examples. I thought it might be best to Perl Info |
From @demerphqOn 13 Sep 2016 14:19, "Todd Rinaldo" <perlbug-followup@perl.org> wrote:
I agree. It looks wrong in several ways.
Not sure what there is to discuss really. Wrong is wrong. Yves
-Wl,-rpath,/usr/local/cpanel/3rdparty/perl/522/lib64/perl5/5.22.1/x86_64-linux-64int/CORE'
/usr/local/cpanel/3rdparty/perl/522/lib64/perl5/cpanel_lib/x86_64-linux-64int
/usr/local/cpanel/3rdparty/perl/522/lib64/perl5/5.22.1/x86_64-linux-64int
PATH=/usr/local/cpanel/3rdparty/perl/524/bin:/usr/local/cpanel/bin:/usr/local/cpanel/3rdparty/bin:/usr/local/cpanel/3rdparty/perl/524/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/cpanel/perl5/524/bin
|
|
The RT System itself - Status changed from 'new' to 'open' |
From @toddrOn Tue Sep 13 11:26:24 2016, demerphq wrote:
Proposed patch. I'm 99% certain we need <= not <. However this is why I use Perl. So I don't have to remember that ;) |
From @toddr0001-Prevent-Perl_gv_fetchmethod_pvn_flags-from-running-p.patchFrom 1c722feb82f0b29d5b439ddc1dd3f05a7a3e115f Mon Sep 17 00:00:00 2001
From: Todd Rinaldo <toddr@cpan.org>
Date: Tue, 13 Sep 2016 13:21:03 -0500
Subject: [PATCH] Prevent Perl_gv_fetchmethod_pvn_flags from running past the
end of a string
Addressed reported issue from Perl #129267
---
gv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gv.c b/gv.c
index 1bc8bf2..c9b706c 100644
--- a/gv.c
+++ b/gv.c
@@ -1028,7 +1028,7 @@ Perl_gv_fetchmethod_pvn_flags(pTHX_ HV *stash, const char *name, const STRLEN le
the error reporting code. */
}
- for (nend = name; *nend || nend != (origname + len); nend++) {
+ for (nend = name; *nend && nend <= (origname + len); nend++) {
if (*nend == '\'') {
nsplit = nend;
name = nend + 1;
--
2.10.0
|
From @petdance
I took the comment to mean “For all I know, there may be other examples elsewhere in the codebase, and it might even be a security hole, but I haven’t investigated further, but someone probably should before we just patch this and call it done." -- |
From @demerphqOn 13 September 2016 at 20:29, Todd Rinaldo via RT
Well, that fixes /one/ issue. But I think there are more. I was $ git diff gv.c Inline Patchdiff --git a/gv.c b/gv.c
index 1bc8bf2..23700a0 100644
--- a/gv.c
+++ b/gv.c
@@ -1009,6 +1009,7 @@ GV *
Perl_gv_fetchmethod_pvn_flags(pTHX_ HV *stash, const char *name,
- for (nend = name; *nend || nend != (origname + len); nend++) { Although I will probably follow it will another patch to rename "nend". Also, somewhat concerning is the comment right above this function: /* Don't merge this yet, as it's likely to get a len parameter, and possibly Sigh. Yves -- |
From @demerphqOn 13 September 2016 at 20:28, Andy Lester <andy@petdance.com> wrote:
Ah, good catch. Well, maybe there is a security hole here, I don't know. But there are a lot of issues with the code as written. In several places It looks to me like if you called this function with a string which ended What would /then/ happen is not clear. Yves -- |
From @toddrOn Tue Sep 13 11:51:18 2016, demerphq wrote:
|
From @demerphqOn 13 September 2016 at 20:29, Todd Rinaldo via RT
FWIW. I dont think so. We need <. Assume name = 0, and len = 10, then we should stop processing when If you use <= then we would try to dereference *10, which we dont own. Yves -- |
From @toddrOn Tue Sep 13 11:29:21 2016, petdance wrote:
Correct. I was a little concerned this code pattern might be being used to walk other string incorrectly. |
From @demerphqOn 13 September 2016 at 20:55, Todd Rinaldo via RT
If its an empty string it should be len == 0. Notice i reversed the clauses as well. We dereference AFTER we have
And again here. We have to recheck things so that we are sure we can nend is a terrible name. Should be name_ptr or something like that. Yves -- |
From @demerphqOn 13 September 2016 at 20:59, Todd Rinaldo via RT
We can and should audit for similar patterns, but my gut feeling is Yves -- |
From @demerphqOn 13 September 2016 at 20:55, Todd Rinaldo via RT
Is that required for some reason I haven't noticed? My version feels Yves |
From @toddrOn Tue Sep 13 12:01:23 2016, demerphq wrote:
S_parse_gv_stash_name is making a similar look ahead mistake with name_cursor[1]. That looks messier to fix but it should probably be another case or a committer should just go through and make the corrections sans perlbug? |
From @demerphqOn 13 September 2016 at 21:06, Todd Rinaldo via RT
You find 'em, and I fix 'em? :-) Yves -- |
From @demerphqOn 13 September 2016 at 21:06, Todd Rinaldo via RT
A quick look didnt reveal to me any issues here. If you look at the Yves -- |
From @cpansproutOn Tue Sep 13 11:54:55 2016, demerphq wrote:
This is an API, function, right? So we can add a test to XS::APItest? -- Father Chrysostomos |
From @demerphqOn 13 September 2016 at 21:29, Father Chrysostomos via RT
I think so yes. FWIW, I accidentally pushed my patches before tests completed, and the Yves -- |
From @toddrOn Tue Sep 13 12:14:27 2016, demerphq wrote:
Yep. Apologies. I pulled the trigger too quick on that one. |
From @demerphqOn 13 September 2016 at 22:00, Todd Rinaldo via RT
Yeah, well so did I with that premature push. :-) Anyway, I repushed my changes: cfb7367 fix #129267: rework gv_fetchmethod_pvn_flags separator parsing I think the code is much easier to understand now. FC, I didnt really see a way to test for a string overrun via (I think this ticket can be closed tho.) Yves -- |
From @toddrOn Tue Sep 13 14:30:52 2016, demerphq wrote:
Looks really good. Thanks! |
From @cpansproutOn Tue Sep 13 14:30:52 2016, demerphq wrote:
Bummer.
The test I added in 1665b71 crashed quite nicely before I rebased on top of your fix.
Done. -- Father Chrysostomos |
|
@cpansprout - Status changed from 'open' to 'pending release' |
From @toddrOn Tue Sep 13 22:44:25 2016, sprout wrote:
Not sure if it's worth it but this backports cleanly to 5.22. Should it be considered for backport? Best I can tell, the backport would be: |
From [Unknown Contact. See original ticket]On Tue Sep 13 22:44:25 2016, sprout wrote:
Not sure if it's worth it but this backports cleanly to 5.22. Should it be considered for backport? Best I can tell, the backport would be: |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release today of Perl 5.26.0, this and 210 other issues have been Perl 5.26.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#129267 (status was 'resolved')
Searchable as RT129267$
The text was updated successfully, but these errors were encountered: