Bug#970096: buster-pu: package libdbi-perl/1.642-1+deb10u1
Le 12/09/2020 à 08:46, Xavier a écrit :
> Le 11/09/2020 à 21:38, Salvatore Bonaccorso a écrit :
>> Hi Xavier,
>>
>> On Fri, Sep 11, 2020 at 06:02:00PM +0200, Xavier Guimard wrote:
>>> Package: release.debian.org
>>> Severity: normal
>>> Tags: buster
>>> User: release.debian.org@packages.debian.org
>>> Usertags: pu
>>> X-Debbugs-Cc: debian-perl@lists.debian.org
>>>
>>> [ Reason ]
>>> libdbi-perl is vulnerable to (low) security bug (CVE-2020-14392)
>>>
>>> [ Impact ]
>>> libdbi-perl may crash if an attacker can give a malformed login
>>>
>>> [ Tests ]
>>> No new test, current passed
>>>
>>> [ Risks ]
>>> This patch is very simple
>>>
>>> [ Checklist ]
>>> [X] *all* changes are documented in the d/changelog
>>> [X] I reviewed all changes and I approve them
>>> [X] attach debdiff against the package in (old)stable
>>> [X] the issue is verified as fixed in unstable
>>>
>>> [ Changes ]
>>> Returned values are more tested
>>
>>> diff --git a/debian/changelog b/debian/changelog
>>> index d2e35cc..d0ad39a 100644
>>> --- a/debian/changelog
>>> +++ b/debian/changelog
>>> @@ -1,3 +1,10 @@
>>> +libdbi-perl (1.642-1+deb10u1) buster; urgency=medium
>>> +
>>> + * Fix memory corruption in XS functions when Perl stack is reallocated
>>> + (Closes: CVE-2020-14392)
>>
>> Note that there is as well CVE-2020-14393, could you add the fix for
>> this one as well?
>
> Thanks for pointing this, here is a new debdiff
Update: typo
diff --git a/debian/changelog b/debian/changelog
index d2e35cc..cd7997e 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,12 @@
+libdbi-perl (1.642-1+deb10u1) buster; urgency=medium
+
+ * Fix memory corruption in XS functions when Perl stack is reallocated
+ (Closes: CVE-2020-14392)
+ * Fix a buffer overflow on an overlong DBD class name
+ (Closes: CVE-2020-14393)
+
+ -- Xavier Guimard <yadd@debian.org> Thu, 10 Sep 2020 10:04:13 +0200
+
libdbi-perl (1.642-1) unstable; urgency=medium
[ Xavier Guimard ]
diff --git a/debian/patches/CVE-2020-14392.patch b/debian/patches/CVE-2020-14392.patch
new file mode 100644
index 0000000..99c7a3e
--- /dev/null
+++ b/debian/patches/CVE-2020-14392.patch
@@ -0,0 +1,318 @@
+Description: Fix memory corruption in XS functions when Perl stack is reallocated
+ Macro ST(*) returns pointer to Perl stack. Other Perl functions which use
+ Perl stack (e.g. eval) may reallocate Perl stack and therefore pointer
+ returned by ST(*) macro is invalid.
+ .
+ Construction like this:
+ .
+ ST(0) = dbd_db_login6_sv(dbh, imp_dbh, dbname, username, password, attribs) ? &PL_sv_yes : &PL_sv_no;
+ .
+ where dbd_db_login6_sv() driver function calls eval may lead to
+ reallocating Perl stack and therefore invalidating ST(0) pointer.
+ So that construction would cause memory corruption as left part of
+ assignment is resolved prior executing dbd_db_login6_sv() function.
+ .
+ Correct way how to handle this problem: First call dbd_db_login6_sv()
+ function and then call ST(0) to retrieve stack pointer.
+ .
+ In this patch are fixes all occurrences of such constructions.
+ .
+ When running perl under valgrind I got memory corruption in DBD::ODBC
+ driver in that dbd_db_login6_sv() function due to above problem.
+Author: Pali <pali@cpan.org>
+Origin: upstream, https://github.com/perl5-dbi/dbi/commit/ea99b6aa
+Bug: https://security-tracker.debian.org/tracker/CVE-2020-14392
+Forwarded: not-needed
+Reviewed-By: Xavier Guimard <yadd@debian.org>
+Last-Update: 2020-09-10
+
+--- a/DBI.xs
++++ b/DBI.xs
+@@ -5252,9 +5252,12 @@
+ SV * col
+ SV * ref
+ SV * attribs
++ PREINIT:
++ SV *ret;
+ CODE:
+ DBD_ATTRIBS_CHECK("bind_col", sth, attribs);
+- ST(0) = boolSV(dbih_sth_bind_col(sth, col, ref, attribs));
++ ret = boolSV(dbih_sth_bind_col(sth, col, ref, attribs));
++ ST(0) = ret;
+ (void)cv;
+
+
+@@ -5492,21 +5495,27 @@
+ FETCH(h, keysv)
+ SV * h
+ SV * keysv
++ PREINIT:
++ SV *ret;
+ CODE:
+- ST(0) = dbih_get_attr_k(h, keysv, 0);
++ ret = dbih_get_attr_k(h, keysv, 0);
++ ST(0) = ret;
+ (void)cv;
+
+ void
+ DELETE(h, keysv)
+ SV * h
+ SV * keysv
++ PREINIT:
++ SV *ret;
+ CODE:
+ /* only private_* keys can be deleted, for others DELETE acts like FETCH */
+ /* because the DBI internals rely on certain handle attributes existing */
+ if (strnEQ(SvPV_nolen(keysv),"private_",8))
+- ST(0) = hv_delete_ent((HV*)SvRV(h), keysv, 0, 0);
++ ret = hv_delete_ent((HV*)SvRV(h), keysv, 0, 0);
+ else
+- ST(0) = dbih_get_attr_k(h, keysv, 0);
++ ret = dbih_get_attr_k(h, keysv, 0);
++ ST(0) = ret;
+ (void)cv;
+
+
+--- a/Driver.xst
++++ b/Driver.xst
+@@ -60,7 +60,7 @@
+ #ifdef dbd_discon_all
+
+ # disconnect_all renamed and ALIAS'd to avoid length clash on VMS :-(
+-void
++bool
+ discon_all_(drh)
+ SV * drh
+ ALIAS:
+@@ -68,7 +68,9 @@
+ CODE:
+ D_imp_drh(drh);
+ PERL_UNUSED_VAR(ix);
+- ST(0) = dbd_discon_all(drh, imp_drh) ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = dbd_discon_all(drh, imp_drh);
++ OUTPUT:
++ RETVAL
+
+ #endif /* dbd_discon_all */
+
+@@ -102,7 +104,7 @@
+ MODULE = DBD::~DRIVER~ PACKAGE = DBD::~DRIVER~::db
+
+
+-void
++bool
+ _login(dbh, dbname, username, password, attribs=Nullsv)
+ SV * dbh
+ SV * dbname
+@@ -118,14 +120,16 @@
+ char *p = (SvOK(password)) ? SvPV(password,lna) : (char*)"";
+ #endif
+ #ifdef dbd_db_login6_sv
+- ST(0) = dbd_db_login6_sv(dbh, imp_dbh, dbname, username, password, attribs) ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = dbd_db_login6_sv(dbh, imp_dbh, dbname, username, password, attribs);
+ #elif defined(dbd_db_login6)
+- ST(0) = dbd_db_login6(dbh, imp_dbh, SvPV_nolen(dbname), u, p, attribs) ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = dbd_db_login6(dbh, imp_dbh, SvPV_nolen(dbname), u, p, attribs);
+ #else
+ PERL_UNUSED_ARG(attribs);
+- ST(0) = dbd_db_login( dbh, imp_dbh, SvPV_nolen(dbname), u, p) ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = dbd_db_login( dbh, imp_dbh, SvPV_nolen(dbname), u, p);
+ #endif
+ }
++ OUTPUT:
++ RETVAL
+
+
+ void
+@@ -296,33 +300,38 @@
+ CODE:
+ {
+ D_imp_dbh(dbh);
+- ST(0) = dbd_db_last_insert_id(dbh, imp_dbh, catalog, schema, table, field, attr);
++ SV *ret = dbd_db_last_insert_id(dbh, imp_dbh, catalog, schema, table, field, attr);
++ ST(0) = ret;
+ }
+
+ #endif
+
+
+-void
++bool
+ commit(dbh)
+ SV * dbh
+ CODE:
+ D_imp_dbh(dbh);
+ if (DBIc_has(imp_dbh,DBIcf_AutoCommit) && DBIc_WARN(imp_dbh))
+ warn("commit ineffective with AutoCommit enabled");
+- ST(0) = dbd_db_commit(dbh, imp_dbh) ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = dbd_db_commit(dbh, imp_dbh);
++ OUTPUT:
++ RETVAL
+
+
+-void
++bool
+ rollback(dbh)
+ SV * dbh
+ CODE:
+ D_imp_dbh(dbh);
+ if (DBIc_has(imp_dbh,DBIcf_AutoCommit) && DBIc_WARN(imp_dbh))
+ warn("rollback ineffective with AutoCommit enabled");
+- ST(0) = dbd_db_rollback(dbh, imp_dbh) ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = dbd_db_rollback(dbh, imp_dbh);
++ OUTPUT:
++ RETVAL
+
+
+-void
++bool
+ disconnect(dbh)
+ SV * dbh
+ CODE:
+@@ -339,8 +348,10 @@
+ SvPV(dbh,lna), (int)DBIc_ACTIVE_KIDS(imp_dbh), plural,
+ "(either destroy statement handles or call finish on them before disconnecting)");
+ }
+- ST(0) = dbd_db_disconnect(dbh, imp_dbh) ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = dbd_db_disconnect(dbh, imp_dbh);
+ DBIc_ACTIVE_off(imp_dbh); /* ensure it's off, regardless */
++ OUTPUT:
++ RETVAL
+
+
+ void
+@@ -474,7 +485,7 @@
+ MODULE = DBD::~DRIVER~ PACKAGE = DBD::~DRIVER~::st
+
+
+-void
++bool
+ _prepare(sth, statement, attribs=Nullsv)
+ SV * sth
+ SV * statement
+@@ -484,11 +495,13 @@
+ D_imp_sth(sth);
+ DBD_ATTRIBS_CHECK("_prepare", sth, attribs);
+ #ifdef dbd_st_prepare_sv
+- ST(0) = dbd_st_prepare_sv(sth, imp_sth, statement, attribs) ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = dbd_st_prepare_sv(sth, imp_sth, statement, attribs);
+ #else
+- ST(0) = dbd_st_prepare(sth, imp_sth, SvPV_nolen(statement), attribs) ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = dbd_st_prepare(sth, imp_sth, SvPV_nolen(statement), attribs);
+ #endif
+ }
++ OUTPUT:
++ RETVAL
+
+
+ #ifdef dbd_st_rows
+@@ -505,7 +518,7 @@
+
+ #ifdef dbd_st_bind_col
+
+-void
++bool
+ bind_col(sth, col, ref, attribs=Nullsv)
+ SV * sth
+ SV * col
+@@ -530,20 +543,21 @@
+ }
+ }
+ switch(dbd_st_bind_col(sth, imp_sth, col, ref, sql_type, attribs)) {
+- case 2: ST(0) = &PL_sv_yes; /* job done completely */
++ case 2: RETVAL = TRUE; /* job done completely */
+ break;
+ case 1: /* fallback to DBI default */
+- ST(0) = (DBIc_DBISTATE(imp_sth)->bind_col(sth, col, ref, attribs))
+- ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = DBIc_DBISTATE(imp_sth)->bind_col(sth, col, ref, attribs);
+ break;
+- default: ST(0) = &PL_sv_no; /* dbd_st_bind_col has called set_err */
++ default: RETVAL = FALSE; /* dbd_st_bind_col has called set_err */
+ break;
+ }
+ }
++ OUTPUT:
++ RETVAL
+
+ #endif /* dbd_st_bind_col */
+
+-void
++bool
+ bind_param(sth, param, value, attribs=Nullsv)
+ SV * sth
+ SV * param
+@@ -567,12 +581,13 @@
+ DBD_ATTRIB_GET_IV(attribs, "TYPE",4, svp, sql_type);
+ }
+ }
+- ST(0) = dbd_bind_ph(sth, imp_sth, param, value, sql_type, attribs, FALSE, 0)
+- ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = dbd_bind_ph(sth, imp_sth, param, value, sql_type, attribs, FALSE, 0);
+ }
++ OUTPUT:
++ RETVAL
+
+
+-void
++bool
+ bind_param_inout(sth, param, value_ref, maxlen, attribs=Nullsv)
+ SV * sth
+ SV * param
+@@ -602,9 +617,10 @@
+ DBD_ATTRIB_GET_IV(attribs, "TYPE",4, svp, sql_type);
+ }
+ }
+- ST(0) = dbd_bind_ph(sth, imp_sth, param, value, sql_type, attribs, TRUE, maxlen)
+- ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = dbd_bind_ph(sth, imp_sth, param, value, sql_type, attribs, TRUE, maxlen);
+ }
++ OUTPUT:
++ RETVAL
+
+
+ void
+@@ -640,7 +656,8 @@
+ CODE:
+ {
+ D_imp_sth(sth);
+- ST(0) = dbd_st_execute_for_fetch(sth, imp_sth, fetch_tuple_sub, tuple_status);
++ SV *ret = dbd_st_execute_for_fetch(sth, imp_sth, fetch_tuple_sub, tuple_status);
++ ST(0) = ret;
+ }
+
+ #endif
+@@ -659,7 +676,8 @@
+ CODE:
+ {
+ D_imp_sth(sth);
+- ST(0) = dbd_st_last_insert_id(sth, imp_sth, catalog, schema, table, field, attr);
++ SV *ret = dbd_st_last_insert_id(sth, imp_sth, catalog, schema, table, field, attr);
++ ST(0) = ret;
+ }
+
+ #endif
+@@ -716,7 +734,7 @@
+ }
+
+
+-void
++bool
+ finish(sth)
+ SV * sth
+ CODE:
+@@ -733,10 +751,12 @@
+ XSRETURN_YES;
+ }
+ #ifdef dbd_st_finish3
+- ST(0) = dbd_st_finish3(sth, imp_sth, 0) ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = dbd_st_finish3(sth, imp_sth, 0);
+ #else
+- ST(0) = dbd_st_finish(sth, imp_sth) ? &PL_sv_yes : &PL_sv_no;
++ RETVAL = dbd_st_finish(sth, imp_sth);
+ #endif
++ OUTPUT:
++ RETVAL
+
+
+ void
diff --git a/debian/patches/CVE-2020-14393.patch b/debian/patches/CVE-2020-14393.patch
new file mode 100644
index 0000000..8b055fe
--- /dev/null
+++ b/debian/patches/CVE-2020-14393.patch
@@ -0,0 +1,89 @@
+Description: Fix a buffer overflow on an overlong DBD class name
+ dbih_setup_handle() in DBI.xs does:
+ .
+ static void
+ dbih_setup_handle(pTHX_ SV *orv, char *imp_class, SV *parent, SV *imp_datasv)
+ {
+ [...]
+ char imp_mem_name[300];
+ [...]
+ strcpy(imp_mem_name, imp_class);
+ strcat(imp_mem_name, "_mem");
+ [...]
+ }
+ .
+ If imp_class argument string value is longer than 300 - strlen("_mem")
+ - 1 bytes, a data will be written past imp_mem_name[] array. The
+ imp_class comes from DBD driver class name (DBI::_new_drh ->
+ _new_handle() -> dbih_setup_handle()).
+ .
+ People usually do not use so long package names (e.g. DBD::ExampleP
+ calls DBI::_new_drh() in lib/DBD/ExampleP.pm), so the risk is low.
+ .
+ Reproducer:
+ .
+ $ perl -MDBI -e 'DBI::_new_drh(q{x} x 300, {}, 0)'
+ *** buffer overflow detected ***: perl terminated
+ Aborted (core dumped)
+ .
+ https://rt.cpan.org/Ticket/Display.html?id=130191
+Author: Petr Pisar <ppisar@redhat.com>
+Origin: upstream, https://github.com/perl5-dbi/dbi/commit/36f2a2c5f
+Bug: https://rt.cpan.org/Ticket/Display.html?id=130191
+Forwarded: not-needed
+Reviewed-By: Xavier Guimard <yadd@debian.org>
+Last-Update: 2020-09-12
+
+--- a/DBI.xs
++++ b/DBI.xs
+@@ -1422,7 +1422,7 @@
+ SV *dbih_imp_rv;
+ SV *dbi_imp_data = Nullsv;
+ SV **svp;
+- char imp_mem_name[300];
++ SV *imp_mem_name;
+ HV *imp_mem_stash;
+ imp_xxh_t *imp;
+ imp_xxh_t *parent_imp;
+@@ -1449,10 +1449,9 @@
+ if (mg_find(SvRV(h), DBI_MAGIC) != NULL)
+ croak(errmsg, neatsvpv(orv,0), imp_class, "already a DBI (or ~magic) handle");
+
+- strcpy(imp_mem_name, imp_class);
+- strcat(imp_mem_name, "_mem");
+- if ( (imp_mem_stash = gv_stashpv(imp_mem_name, FALSE)) == NULL)
+- croak(errmsg, neatsvpv(orv,0), imp_mem_name, "unknown _mem package");
++ imp_mem_name = sv_2mortal(newSVpvf("%s_mem", imp_class));
++ if ( (imp_mem_stash = gv_stashsv(imp_mem_name, FALSE)) == NULL)
++ croak(errmsg, neatsvpv(orv,0), SvPVbyte_nolen(imp_mem_name), "unknown _mem package");
+
+ if ((svp = hv_fetch((HV*)SvRV(h), "dbi_imp_data", 12, 0))) {
+ dbi_imp_data = *svp;
+--- a/t/02dbidrv.t
++++ b/t/02dbidrv.t
+@@ -4,7 +4,7 @@ $|=1;
+
+ use strict;
+
+-use Test::More tests => 53;
++use Test::More tests => 54;
+
+ ## ----------------------------------------------------------------------------
+ ## 02dbidrv.t - ...
+@@ -21,6 +21,16 @@
+ use_ok('DBI');
+ }
+
++## DBI::_new_drh had an internal limit on a driver class name and crashed.
++SKIP: {
++ Test::More::skip "running DBI::PurePerl", 1 if $DBI::PurePerl;
++ eval {
++ DBI::_new_drh('DBD::Test::OverLong' . 'x' x 300,
++ { Name => 'Test', Version => 'Test', }, 42);
++ };
++ like($@, qr/unknown _mem package/, 'Overlong DBD class name is processed');
++}
++
+ ## ----------------------------------------------------------------------------
+ ## create a Test Driver (DBD::Test)
+
diff --git a/debian/patches/series b/debian/patches/series
index 3a41634..a188c1a 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -2,3 +2,5 @@ t__06attrs.t__localefix.patch
t__40profile.t__NTP.patch
t__80proxy.t___syslogd.patch
spelling.patch
+CVE-2020-14392.patch
+CVE-2020-14393.patch
diff --git a/debian/patches/spelling.patch b/debian/patches/spelling.patch
index 2ffb778..8874ab9 100644
--- a/debian/patches/spelling.patch
+++ b/debian/patches/spelling.patch
@@ -7,7 +7,7 @@ Bug: https://rt.cpan.org/Ticket/Display.html?id=114066
--- a/DBI.pm
+++ b/DBI.pm
-@@ -7884,7 +7884,7 @@
+@@ -7905,7 +7905,7 @@
$self->{_buf} .= shift;
#
# DBI feeds us pieces at a time, so accumulate a complete line
Reply to: