[Bug dynamic-link/26634] New: ld.so stats raw file names, bypassing the audit module

[Bug dynamic-link/26634] New: ld.so stats raw file names, bypassing the audit module

[ludo at gnu dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

            Bug ID: 26634
           Summary: ld.so stats raw file names, bypassing the audit module
           Product: glibc
           Version: 2.31
            Status: NEW
          Severity: normal
          Priority: P2
         Component: dynamic-link
          Assignee: unassigned at sourceware dot org
          Reporter: ludo at gnu dot org
  Target Milestone: ---

Created attachment 12850
  --> https://sourceware.org/bugzilla/attachment.cgi?id=12850&action=edit
Audit module that can be used as a reproducer

Unless I'm mistaken, the loader can end up stat'ing raw directory names
(DT_RUNPATH entries) instead of stat'ing names returned by the audit module's
'la_objsearch'.

The attached audit module illustrates that.  It's supposed to add "/PREFIX"
anytime 'name' has a leading slash (typically LA_SER_RUNPATH); yet, strace'ing
it shows that it calls 'stat' on the original file name, without "/PREFIX":

> $ strace -E LD_AUDIT=$PWD/audit.so -e stat expr --version
> [...]
> la_objsearch 4 '/gnu/store/35afkywncrr5xsb4cxcljf6rpjcb7f61-gmp-6.2.0/lib/haswell/libgmp.so.10' -> '/PREFIX/gnu/store/35afkywncrr5xsb4cxcljf6rpjcb7f61-gmp-6.2.0/lib/haswell/libgmp.so.10'
> stat("/gnu/store/35afkywncrr5xsb4cxcljf6rpjcb7f61-gmp-6.2.0/lib/haswell", 0x7ffe9f84c240) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
> la_objsearch 4 '/gnu/store/35afkywncrr5xsb4cxcljf6rpjcb7f61-gmp-6.2.0/lib/x86_64/libgmp.so.10' -> '/PREFIX/gnu/store/35afkywncrr5xsb4cxcljf6rpjcb7f61-gmp-6.2.0/lib/x86_64/libgmp.so.10'
> stat("/gnu/store/35afkywncrr5xsb4cxcljf6rpjcb7f61-gmp-6.2.0/lib/x86_64", 0x7ffe9f84c240) = -1 ENOENT (Dosiero aŭ dosierujo ne ekzistas)
> la_objsearch 4 '/gnu/store/35afkywncrr5xsb4cxcljf6rpjcb7f61-gmp-6.2.0/lib/libgmp.so.10' -> '/PREFIX/gnu/store/35afkywncrr5xsb4cxcljf6rpjcb7f61-gmp-6.2.0/lib/libgmp.so.10'
> stat("/gnu/store/35afkywncrr5xsb4cxcljf6rpjcb7f61-gmp-6.2.0/lib", {st_mode=S_IFDIR|0555, st_size=4096, ...}) = 0
> [...]

(Here the /gnu/store directory is from expr's DT_RUNPATH; notice that 'stat'
ignores "/PREFIX".)

The consequence is that some search path entries get marked as 'nonexisting'
even though they potentially shouldn't.

The culprit appears to be 'open_path' in 'dl-load.c':

          fd = open_verify (buf, -1, fbp, loader, whatcode, mode,
                            found_other_class, false);
          if (this_dir->status[cnt] == unknown)
            {
              if (fd != -1)
                this_dir->status[cnt] = existing;
              /* Do not update the directory information when loading
                 auditing code.  We must try to disturb the program as
                 little as possible.  */
              else if (loader == NULL
                       || GL(dl_ns)[loader->l_ns]._ns_loaded->l_auditing == 0)
                {
                  /* We failed to open machine dependent library.  Let's
                     test whether there is any directory at all.  */
                  struct stat64 st;

                  buf[buflen - namelen - 1] = '\0';

                  if (__xstat64 (_STAT_VER, buf, &st) != 0
                      || ! S_ISDIR (st.st_mode))
                    /* The directory does not exist or it is no directory.  */
                    this_dir->status[cnt] = nonexisting;
                  else
                    this_dir->status[cnt] = existing;
                }
            }

Here 'buf' contains the original name.  'open_verify' is passed that original
name, calls the audit module's 'objsearch' function, and works on that. 
However, the '__xstat64' call right below is passed 'buf'--i.e., the original
name.

Does that make sense?

(This stems from <https://issues.guix.gnu.org/43491>.)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug dynamic-link/26634] ld.so stats raw file names, bypassing the audit module

[mark at klomp dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mark at klomp dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug dynamic-link/26634] ld.so stats raw file names, bypassing the audit module

[ludo at gnu dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

--- Comment #1 from Ludovic Courtès <ludo at gnu dot org> ---
Created attachment 12861
  --> https://sourceware.org/bugzilla/attachment.cgi?id=12861&action=edit
Patch to skip 'stat' when the audit module provided a different file name

This attached patch provides a fix/workaround: it leaves directory information
unchanged (skips the incorrect 'stat' calls) when 'open_verify' returns -1 and
'changed_by_audit == true'.

I confirm that it solves the initial case reported at
<https://issues.guix.gnu.org/43491>.

I this approach sounds reasonable, I can formally submit it to libc-alpha--let
me know!

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug dynamic-link/26634] ld.so stats raw file names, bypassing the audit module

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |security-
                 CC|                            |fweimer at redhat dot com

--- Comment #2 from Florian Weimer <fweimer at redhat dot com> ---
Is there an actual bug here? The loader caches information that search path
entries do not exist. It is likely that this information will be useful for
further library loading. So this does not even seem to be a performance bug.

Or is there an observable behavioral difference?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug dynamic-link/26634] ld.so stats raw file names, bypassing the audit module

[ludo at gnu dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

--- Comment #3 from Ludovic Courtès <ludo at gnu dot org> ---
(In reply to Florian Weimer from comment #2)
> Is there an actual bug here? The loader caches information that search path
> entries do not exist. It is likely that this information will be useful for
> further library loading. So this does not even seem to be a performance bug.
> 
> Or is there an observable behavioral difference?

Yes: when an audit module is used, the loader potentially stats the wrong
directory (the "original" one instead of that returned by the audit module),
and thus caches wrong information about search path entries.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug dynamic-link/26634] ld.so stats raw file names, bypassing the audit module

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

--- Comment #4 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Ludovic Courtès from comment #3)
> (In reply to Florian Weimer from comment #2)
> > Is there an actual bug here? The loader caches information that search path
> > entries do not exist. It is likely that this information will be useful for
> > further library loading. So this does not even seem to be a performance bug.
> > 
> > Or is there an observable behavioral difference?
> 
> Yes: when an audit module is used, the loader potentially stats the wrong
> directory (the "original" one instead of that returned by the audit module),
> and thus caches wrong information about search path entries.

Sorry, since the search path contains only the name without /PREFIX, probing
without /PREFIX seems like the right thing to do to me. What am I missing?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug dynamic-link/26634] ld.so stats raw file names, bypassing the audit module

[ludo at gnu dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

--- Comment #5 from Ludovic Courtès <ludo at gnu dot org> ---
(In reply to Florian Weimer from comment #4)
> Sorry, since the search path contains only the name without /PREFIX, probing
> without /PREFIX seems like the right thing to do to me. What am I missing?

'open_verify' checks /PREFIX/xyz/libfoo.so and returns -1.  Then the caller
stats /xyz, determines that it's ENOENT, and marks the entry as nonexisting.

This is inconsistent: /PREFIX/xyz may well exist and thus, had we statted it,
we would not have marked the entry as nonexisting.

Does that make sense?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug dynamic-link/26634] ld.so stats raw file names, bypassing the audit module

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

--- Comment #6 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Ludovic Courtès from comment #5)
> (In reply to Florian Weimer from comment #4)
> > Sorry, since the search path contains only the name without /PREFIX, probing
> > without /PREFIX seems like the right thing to do to me. What am I missing?
> 
> 'open_verify' checks /PREFIX/xyz/libfoo.so and returns -1.  Then the caller
> stats /xyz, determines that it's ENOENT, and marks the entry as nonexisting.
> 
> This is inconsistent: /PREFIX/xyz may well exist and thus, had we statted
> it, we would not have marked the entry as nonexisting.

I assume (based on my limited understanding of the code), that it stats /xyz
and marks /xyz as non-existing.  The search paths do not contain /PREFIX
because that was added by la_objsearch.  open_verify does not change the
incoming pathname buffer.

Are you observing something different?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug dynamic-link/26634] ld.so stats raw file names, bypassing the audit module

[ludo at gnu dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

--- Comment #7 from Ludovic Courtès <ludo at gnu dot org> ---
(In reply to Florian Weimer from comment #6)
> I assume (based on my limited understanding of the code), that it stats /xyz
> and marks /xyz as non-existing.  The search paths do not contain /PREFIX
> because that was added by la_objsearch.  open_verify does not change the
> incoming pathname buffer.
> 
> Are you observing something different?

No, what you're writing is correct.

However, the effect is that /PREFIX/xyz will never be searched again because of
this inconsistency, even though it possibly should.  To me this is a bug
because the audit module's rewrite is not honored.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug dynamic-link/26634] ld.so stats raw file names, bypassing the audit module

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

--- Comment #8 from Florian Weimer <fweimer at redhat dot com> ---
Sorry, this is not what I meant. I expect that /xyz will never be searched
again. But that is fine, because it does not actually exist?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug dynamic-link/26634] ld.so stats raw file names, bypassing the audit module

[ludo at gnu dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

--- Comment #9 from Ludovic Courtès <ludo at gnu dot org> ---
(In reply to Florian Weimer from comment #8)
> Sorry, this is not what I meant. I expect that /xyz will never be searched
> again. But that is fine, because it does not actually exist?

To me it's not OK: the audit module said "look under /PREFIX/xyz" so what
matters is whether /PREFIX/xyz exists, not whether /xyz exists.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug dynamic-link/26634] ld.so stats raw file names, bypassing the audit module

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

--- Comment #10 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Ludovic Courtès from comment #9)
> To me it's not OK: the audit module said "look under /PREFIX/xyz" so what
> matters is whether /PREFIX/xyz exists, not whether /xyz exists.

But there is no search path entry for /PREFIX/xyz, so there is nothing to cache
there. Let me think about it.

Maybe we should disable negative caching if la_objsearch is present altogether.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug dynamic-link/26634] ld.so stats raw file names, bypassing the audit module

[ludo at gnu dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

--- Comment #11 from Ludovic Courtès <ludo at gnu dot org> ---
(In reply to Florian Weimer from comment #10)
> (In reply to Ludovic Courtès from comment #9)
> > To me it's not OK: the audit module said "look under /PREFIX/xyz" so what
> > matters is whether /PREFIX/xyz exists, not whether /xyz exists.
> 
> But there is no search path entry for /PREFIX/xyz, so there is nothing to
> cache there. Let me think about it.
> 
> Maybe we should disable negative caching if la_objsearch is present
> altogether.

Yes, I agree.  The patch I attached earlier does that.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug dynamic-link/26634] ld.so stats raw file names, bypassing the audit module

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26634

--- Comment #12 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Ludovic Courtès from comment #11)
> > Maybe we should disable negative caching if la_objsearch is present
> > altogether.
> 
> Yes, I agree.  The patch I attached earlier does that.

Only if la_objsearch changed the path. I was thinking about doing it more
generally.

-- 
You are receiving this mail because:
You are on the CC list for the bug.