rtld: Reject overly long LD_AUDIT path elements
Commit Message
On 06/26/2017 02:57 PM, Andreas Schwab wrote:
> On Jun 26 2017, Florian Weimer <fweimer@redhat.com> wrote:
>
>> The goal is to prevent massaging the heap through LD_AUDIT variable
>> contents. So it's purely hardening.
>
> Why is that needed?
I'm not sure if it is needed. I am not an experienced exploit writer.
I assume you want me to apply something like the attached patch, right?
Thanks,
Florian
Comments
On Thu, Jun 29, 2017 at 3:05 PM, Florian Weimer <fweimer@redhat.com> wrote:
> On 06/26/2017 02:57 PM, Andreas Schwab wrote:
>> On Jun 26 2017, Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> The goal is to prevent massaging the heap through LD_AUDIT variable
>>> contents. So it's purely hardening.
>>
>> Why is that needed?
>
> I'm not sure if it is needed. I am not an experienced exploit writer.
>
> I assume you want me to apply something like the attached patch, right?
I am not an experienced exploit writer either, and I don't know this
code at all, but as a matter of principle, I do not think you should
make any changes until Andreas actually explains his concerns in
_detail_. One-sentence cryptic questions, at a rate of one per email,
are not proper code review.
zw
On 06/29/2017 09:56 PM, Zack Weinberg wrote:
> On Thu, Jun 29, 2017 at 3:05 PM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 06/26/2017 02:57 PM, Andreas Schwab wrote:
>>> On Jun 26 2017, Florian Weimer <fweimer@redhat.com> wrote:
>>>
>>>> The goal is to prevent massaging the heap through LD_AUDIT variable
>>>> contents. So it's purely hardening.
>>>
>>> Why is that needed?
>>
>> I'm not sure if it is needed. I am not an experienced exploit writer.
>>
>> I assume you want me to apply something like the attached patch, right?
>
> I am not an experienced exploit writer either, and I don't know this
> code at all, but as a matter of principle, I do not think you should
> make any changes until Andreas actually explains his concerns in
> _detail_. One-sentence cryptic questions, at a rate of one per email,
> are not proper code review.
To be fair, the original patch went in without much review on
libc-alpha, too.
Florian
On Thu, Jun 29, 2017 at 4:36 PM, Florian Weimer <fweimer@redhat.com> wrote:
> On 06/29/2017 09:56 PM, Zack Weinberg wrote:
>>
>> I am not an experienced exploit writer either, and I don't know this
>> code at all, but as a matter of principle, I do not think you should
>> make any changes until Andreas actually explains his concerns in
>> _detail_. One-sentence cryptic questions, at a rate of one per email,
>> are not proper code review.
>
> To be fair, the original patch went in without much review on
> libc-alpha, too.
Carlos made clear that he had, in fact, read the entire thing, and I
trust that if he had had concerns he would have said so.
zw
On 06/29/2017 05:09 PM, Zack Weinberg wrote:
> On Thu, Jun 29, 2017 at 4:36 PM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 06/29/2017 09:56 PM, Zack Weinberg wrote:
>>>
>>> I am not an experienced exploit writer either, and I don't know this
>>> code at all, but as a matter of principle, I do not think you should
>>> make any changes until Andreas actually explains his concerns in
>>> _detail_. One-sentence cryptic questions, at a rate of one per email,
>>> are not proper code review.
>>
>> To be fair, the original patch went in without much review on
>> libc-alpha, too.
>
> Carlos made clear that he had, in fact, read the entire thing, and I
> trust that if he had had concerns he would have said so.
I read the whole thing, and multiple times. I had no concerns with the
patch. Is it exactly how I would have solved it? No. However, my
responsibility as a reviewer is not to make everyone conform to the
way *I* would have done it. In fact I appreciate immensely that Florian
comes up with answers that I didn't think about, because it adds
resilience to our community.
If there is a defect in the original patch I didn't detect it in my
review, and I reviewed over a dozen backports of the same patch. I think
I could rewrite it by hand without looking at the original.
The only overarching concern for *all* the patches is that we previously
had no limits, which is what the CVE is about, and now we *do* have limits
and that's because we need them to protect against this attack vector.
Eventually I forsee with -fstack-check we might lift the limits again,
but until then we need to limit exactly what secure applications can do.
ld.so: Simplify processing of LD_AUDIT
Return to the old scheme of calling process_dl_audit and allocating
the audit list on the heap, but process the last environment variable
only.
This reverts most of commit 81b82fb966ffbd94353f793ad17116c6088dedd9.
2017-06-29 Florian Weimer <fweimer@redhat.com>
* elf/rtld.c (process_dl_audit): Call dso_name_valid_for_suid.
(audit_list_string): Remove variable.
(struct audit_list_iter): Remove definition.
(audit_list_iter_init, audit_list_iter_next): Remove functions.
(dl_main): Remove iterator code and iterate over audit_list
directly.
(process_envvars): Process LD_AUDIT once using process_dl_audit.
@@ -129,10 +129,6 @@ dso_name_valid_for_suid (const char *p)
return *p != '\0';
}
-/* LD_AUDIT variable contents. Must be processed before the
- audit_list below. */
-const char *audit_list_string;
-
/* Cyclic list of auditing DSOs. audit_list->next is the first
element. */
static struct audit_list
@@ -141,79 +137,6 @@ static struct audit_list
struct audit_list *next;
} *audit_list;
-/* Iterator for audit_list_string followed by audit_list. */
-struct audit_list_iter
-{
- /* Tail of audit_list_string still needing processing, or NULL. */
- const char *audit_list_tail;
-
- /* The list element returned in the previous iteration. NULL before
- the first element. */
- struct audit_list *previous;
-
- /* Scratch buffer for returning a name which is part of
- audit_list_string. */
- char fname[SECURE_NAME_LIMIT];
-};
-
-/* Initialize an audit list iterator. */
-static void
-audit_list_iter_init (struct audit_list_iter *iter)
-{
- iter->audit_list_tail = audit_list_string;
- iter->previous = NULL;
-}
-
-/* Iterate through both audit_list_string and audit_list. */
-static const char *
-audit_list_iter_next (struct audit_list_iter *iter)
-{
- if (iter->audit_list_tail != NULL)
- {
- /* First iterate over audit_list_string. */
- while (*iter->audit_list_tail != '\0')
- {
- /* Split audit list at colon. */
- size_t len = strcspn (iter->audit_list_tail, ":");
- if (len > 0 && len < sizeof (iter->fname))
- {
- memcpy (iter->fname, iter->audit_list_tail, len);
- iter->fname[len] = '\0';
- }
- else
- /* Do not return this name to the caller. */
- iter->fname[0] = '\0';
-
- /* Skip over the substring and the following delimiter. */
- iter->audit_list_tail += len;
- if (*iter->audit_list_tail == ':')
- ++iter->audit_list_tail;
-
- /* If the name is valid, return it. */
- if (dso_name_valid_for_suid (iter->fname))
- return iter->fname;
- /* Otherwise, wrap around and try the next name. */
- }
- /* Fall through to the procesing of audit_list. */
- }
-
- if (iter->previous == NULL)
- {
- if (audit_list == NULL)
- /* No pre-parsed audit list. */
- return NULL;
- /* Start of audit list. The first list element is at
- audit_list->next (cyclic list). */
- iter->previous = audit_list->next;
- return iter->previous->name;
- }
- if (iter->previous == audit_list)
- /* Cyclic list wrap-around. */
- return NULL;
- iter->previous = iter->previous->next;
- return iter->previous->name;
-}
-
#ifndef HAVE_INLINED_SYSCALLS
/* Set nonzero during loading and initialization of executable and
libraries, cleared before the executable's entry point runs. This
@@ -1383,13 +1306,11 @@ of this helper program; chances are you did not intend to run this program.\n\
GL(dl_rtld_map).l_tls_modid = _dl_next_tls_modid ();
/* If we have auditing DSOs to load, do it now. */
- bool need_security_init = true;
- if (__glibc_unlikely (audit_list != NULL)
- || __glibc_unlikely (audit_list_string != NULL))
+ if (__glibc_unlikely (audit_list != NULL))
{
+ /* Iterate over all entries in the list. The order is important. */
struct audit_ifaces *last_audit = NULL;
- struct audit_list_iter al_iter;
- audit_list_iter_init (&al_iter);
+ struct audit_list *al = audit_list->next;
/* Since we start using the auditing DSOs right away we need to
initialize the data structures now. */
@@ -1400,14 +1321,9 @@ of this helper program; chances are you did not intend to run this program.\n\
use different values (especially the pointer guard) and will
fail later on. */
security_init ();
- need_security_init = false;
- while (true)
+ do
{
- const char *name = audit_list_iter_next (&al_iter);
- if (name == NULL)
- break;
-
int tls_idx = GL(dl_tls_max_dtv_idx);
/* Now it is time to determine the layout of the static TLS
@@ -1416,7 +1332,7 @@ of this helper program; chances are you did not intend to run this program.\n\
no DF_STATIC_TLS bit is set. The reason is that we know
glibc will use the static model. */
struct dlmopen_args dlmargs;
- dlmargs.fname = name;
+ dlmargs.fname = al->name;
dlmargs.map = NULL;
const char *objname;
@@ -1429,7 +1345,7 @@ of this helper program; chances are you did not intend to run this program.\n\
not_loaded:
_dl_error_printf ("\
ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
- name, err_str);
+ al->name, err_str);
if (malloced)
free ((char *) err_str);
}
@@ -1533,7 +1449,10 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
goto not_loaded;
}
}
+
+ al = al->next;
}
+ while (al != audit_list->next);
/* If we have any auditing modules, announce that we already
have two objects loaded. */
@@ -1797,7 +1716,7 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
if (tcbp == NULL)
tcbp = init_tls ();
- if (__glibc_likely (need_security_init))
+ if (__glibc_likely (audit_list == NULL))
/* Initialize security features. But only if we have not done it
earlier. */
security_init ();
@@ -2458,6 +2377,7 @@ process_envvars (enum mode *modep)
char *envline;
enum mode mode = normal;
char *debug_output = NULL;
+ char *audit_list_string = NULL;
/* This is the default place for profiling data file. */
GLRO(dl_profile_output)
@@ -2623,6 +2543,9 @@ process_envvars (enum mode *modep)
/* The caller wants this information. */
*modep = mode;
+ if (audit_list_string != NULL)
+ process_dl_audit (audit_list_string);
+
/* Extra security for SUID binaries. Remove all dangerous environment
variables. */
if (__builtin_expect (__libc_enable_secure, 0))