Hello Nick,

this is another component where we see the stack-prot test to be skipped. Could you provide your expert opinion about the source of this case?

Thank you, Jan

Checking with

readelf -Ws /usr/lib64/libicudata.so.67.1 | grep '__stack_chk_fail'

does not find anything, meaning the function to be called when the stack protection detects a corrupted stack is not referenced. So it seems to suggest -fstack-protector-strong was indeed not used.

(In reply to Jan Pazdziora from comment #2)

> this is another component where we see the stack-prot test to be skipped.
> Could you provide your expert opinion about the source of this case?

The same as the others I suspect - the code is being compiled without annobin annotation and without the security hardening options enabled.

Nick, Steve Grubb notes:

> I think this library is pure data. There doesn't seem to be any functions found by readelf. Instead it has NOTYPE and OBJECT rather than FUNC.

Checking the build.log of that rpm, it has

gcc -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -m64 -march=x86-64-v2 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fno-strict-aliasing -std=c11 -Wall -pedantic -Wshadow -Wpointer-arith -Wmissing-prototypes -Wwrite-strings   -Wl,-z,relro -Wl,--as-needed  -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1    -shared -Wl,-Bsymbolic -Wl,-soname -Wl,libicudata.so.67 -o libicudata.so.67.1 stubdata.o 

and -fstack-protector-strong is there. So it looks like this component does everything in its power to compile correctly.

Is it correct that in case there are no functions in the resulting .so, the annobin plugin does not add the annotations? Or that the debug file does not get generated properly as the message

Hardened: libicudata.so.67.1: warn: Failed to parse separate debug file '/usr/lib/debug/.build-id/a9/80f32bc1fc2ee613ed6123767f7432721156e6.debug', (no DWARF information).

would suggest?

IOW, is this one potentially a real annobin problem?

(In reply to Jan Pazdziora from comment #6)

> > I think this library is pure data. There doesn't seem to be any functions found by readelf. Instead it has NOTYPE and OBJECT rather than FUNC.

A library that is pure data ?  Strange.  But permitted I guess.


 
> Is it correct that in case there are no functions in the resulting .so, the
> annobin plugin does not add the annotations?

Yes.  Annobin only annotates code not data.  (As far as I know there are no 
compile time options that affect the security of data).

> Or that the debug file does not
> get generated

Right - no code = no debug info.

 
> IOW, is this one potentially a real annobin problem?

Not really.  I think that this is one case where an exception (in the icu package's rpmimnspect.yaml file) is justified.  Since the library contains no code, running code specific checks on it, such as stack protection, is unwarranted.  

Annocheck does correctly report that it cannot find evidence of compiled code, and it does not flag the file as failing the stack protection test, so I do not think that there is an annobin issue here.

(In reply to Nick Clifton from comment #7)
> (In reply to Jan Pazdziora from comment #6)
>  
> > Is it correct that in case there are no functions in the resulting .so, the
> > annobin plugin does not add the annotations?
> 
> Yes.  Annobin only annotates code not data.  (As far as I know there are no 
> compile time options that affect the security of data).
>
> > Or that the debug file does not
> > get generated
> 
> Right - no code = no debug info.

Could the behaviour of annobin be extended to add annotation of data? Not for security per se but to produce consistent debuginfo experience for the tools consuming the results, like annocheck.

Alternatively, could annocheck detect that there are no code segments and skip the check cleanly, ideally with a message that would be explicit about the reason? The current message

  skip: stack-prot test because no compiled code found

can mean both "there is no code, just data", and "something might be wrong, maybe we don't have debuginfo, could not find any code".

> Annocheck does correctly report that it cannot find evidence of compiled
> code, and it does not flag the file as failing the stack protection test, so
> I do not think that there is an annobin issue here.

The problem is, annocheck does not flag the file (other files) as failing even in situations when there is a code and annocheck just did not find it for example due to a missing debuginfo. Annocheck's focus seems to be to avoid false reports, and rather skip than report a fail.

For certification reasons we might need to know when annocheck is sure there is no code, vs. annocheck just couldn't find the code.

(In reply to Jan Pazdziora from comment #8)
 
>> Right - no code = no debug info.
> 
> Could the behaviour of annobin be extended to add annotation of data? Not
> for security per se but to produce consistent debuginfo experience for the
> tools consuming the results, like annocheck.

It could, but it would not really change anything.  Annocheck is not interested
in the data of a program(*) so even if there were annobin notes covering the data
annocheck would not process them.

Plus I should note that annobin notes are not the same thing as debug information,
so you can have programs with debug info but without annobin notes and vice versa.

In fact if you are being strict about it, it is possible to generate debug info
for data-only binaries like libicudata, since DWARF can describe things like the
data types, sizes, names, etc.  So when I said "no code = no debug info" above,
that was not strictly correct.

(*) Whilst annocheck does not test the data of a binary, it does check to make
sure that none of the segments have both write and execute permissions.  Normally
code segments have read+execute and data segments have read+write (or just read 
for read only data).  But no segment should have both the write and execute flags
set.


 
> Alternatively, could annocheck detect that there are no code segments and
> skip the check cleanly, ideally with a message that would be explicit about
> the reason? 

Ah - now that is a very good idea.  I will add that to annocheck.

 
 
> The problem is, annocheck does not flag the file (other files) as failing
> even in situations when there is a code and annocheck just did not find it
> for example due to a missing debuginfo. Annocheck's focus seems to be to
> avoid false reports, and rather skip than report a fail.
> 
> For certification reasons we might need to know when annocheck is sure there
> is no code, vs. annocheck just couldn't find the code.

Ok, so your idea above should solve that issue.  If there are no code segments
in the executable then annocheck can PASS the stack prot test with a message
along the lines of "no code present therefore no stack protection is needed".

(In reply to Jan Pazdziora from comment #6)
> Nick, Steve Grubb notes:
> 
>> I think this library is pure data. There doesn't seem to be any functions found by readelf. Instead it has NOTYPE and OBJECT rather than FUNC.

Looking into this further (in an attempt to enhance annocheck's ability to detect binaries that do not contain code) I found that whilst there are no FUNC type symbols the library *does* contain a .text section and it does have an executable segment.  So it looks like annocheck is going to have to read and parse the entire symbol table of a binary before it can detect that it does not contain code...

Fixed in gcc-toolset-12-annobin-10.94-1.el9

Note - this build comes from fixing another BZ, but it also includes the fix for this issue as well.

Waiting for an ITM so that I can move the BZ into the MODIFIED state.

Fixed in gcc-toolset-12-annobin-10.94-1.el9

(In reply to Jan Pazdziora from comment #20)
Hi Jan,

> FAIL /annocheck/usr/sbin/thin_metadata_unpack::skip: stack-prot test because
> no code present - therefore no stack protection needed
 
> What concerns me the most are the tracepath to arping outputs that are now
> reported as "no code present - therefore test not needed". But in bug
> 2144509 it was determined that they were indeed missing the standard
> compiler flags and that a fix was needed. So it seems that the logic that
> should kick in only when annocheck should be really really sure that there
> is no faulty code is also applied in these cases when a faulty code is
> there, making the change less than useful.
 
Hmm, you are right, arping (and the other executables in the iputils rpm) are very suspicious.  They appear to contain code, but no function symbols of any kind.  I think that annocheck needs to extend its heuristic to complain if the .text section is suspiciously large when symbols are missing.

What do you think of this change:

  % annocheck -n --skip-all --test-stack-prot iputils-20210202-8.el9.x86_64.rpm libicu-67.1-9.el9.x86_64.rpm
annocheck: Version 10.96.
Hardened: using profile: none.
Hardened: arping: WARN: large text section with no function symbols detected
Hardened: arping: WARN: this suggests compiled code and a stripped symbol table
Hardened: arping: MAYB: test: stack-prot because no notes found regarding this feature 
Hardened: arping: Overall: FAIL (due to MAYB results).
[...]
Hardened: libicudata.so.67.1: WARN: If this file was built from assembler sources then it may need updating to support stack protection
Hardened: libicudata.so.67.1: WARN: For more details see https://sourceware.org/annobin/annobin.html/Absence-of-compiled-code.html
Hardened: libicudata.so.67.1: PASS.
[...]

So arping triggers a FAIL result because it has a large .text section, despite not having any function symbols.  Whereas libicudata.so.67.1 does not trigger a FAIL result.  It too does not have any function symbols, but its .text section is small.

Cheers
  Nick

PS.  I think that you can use "brew untag annobin-10.94-1.el9" to remove the update from the buildroot.  Although if you like the above suggestion I can probably have the new version in the buildroot tomorrow...

Could you do a (scratch) build of that 10.96 version? I'd do a quick check in our CI to get results across the wider set of cases.

Hi Jan,

Here you go:

 https://kojihub.stream.rdu2.redhat.com/koji/taskinfo?taskID=1692170

Yeah - that version is not right.  I am working on another one...

OK, here is a better version:

https://kojihub.stream.rdu2.redhat.com/koji/taskinfo?taskID=1693312

With this version I get these results:

  % annocheck -n --skip-all -test-stack-prot libicu-67.1-9.el9.x86_64.rpm --verbose
  annocheck: Version 10.96.
  Hardened: using profile: none.
  Hardened: ./usr/lib64/libicudata.so.67.1: skip: stack-prot test because no code present - therefore no stack protection needed 
  Hardened: ./usr/lib64/libicudata.so.67.1: Overall: PASS.
  [...]
 
So annocheck correctly worked out that libicudata.so.67.1 does not contain any code.
Then checking the old iputils rpm:

  % annocheck -n --skip-all -test-stack-prot iputils-20210202-7.el9.x86_64.rpm --verbose
  annocheck: Version 10.96.
  Hardened: using profile: none.
  Hardened: ./usr/bin/arping: skip: stack-prot test because built from assembler sources 
  Hardened: ./usr/bin/arping: WARN: The assembler sources may need updating to support stack protection
  Hardened: ./usr/bin/arping: WARN: For more details see https://sourceware.org/annobin/annobin.html/Absence-of-compiled-code.html
  Hardened: ./usr/bin/arping: Overall: PASS.
  [...]

So not an ideal result.  Annocheck has failed to detect that arping is built from compiled C, but it has at least generated a warning message suggesting that since it contains code, it must be assembler and that assembler needs to be checked.

Checking the new iputils rpm produces the correct result, but by accident:

  % annocheck -n --skip-all -test-stack-prot iputils-20210202-8.el9.x86_64.rpm --verbose
  annocheck: Version 10.96.
  Hardened: using profile: none.
  Hardened: ./usr/bin/arping: skip: stack-prot test because not compiled from C/C++ 
  Hardened: ./usr/bin/arping: Overall: PASS.

Annocheck still does not know that arping is a compiled program.  But this time it is not even sure if assembler is involved, so it just punts.

But, if you give annocheck access to the debuginfo files as well:

  % annocheck -n --skip-all -test-stack-prot iputils-20210202-8.el9.x86_64.rpm --verbose --debug-rpm iputils-debuginfo-20210202-8.el9.x86_64.rpm
  annocheck: Version 10.96.
  Hardened: using profile: none.
  Hardened: ./usr/bin/arping: PASS: stack-prot test 

So now annocheck has determined that the stack-protection option was used (based upon information in the DWARF debug section), and it correctly PASSes the test.  

Note - there is still an issue in that the 20210202-8.el9 rpms are built without enabling annobin annotation, so annocheck is still not going to be able to due a thorough job.

Providing annocheck with access to the debuginfo files for the 20210202-7.el9 build does not help, because those builds did not actually generate any debug information. :-(

------

I am not sure how to make annocheck better at determining if a file was compiled.  Checking for a large .text section and no function symbols proved to be a non-starter.  Most compiled programs (as opposed to shared libaries) fall into this category.

Any suggestions ?

I checked https://kojihub.stream.rdu2.redhat.com/koji/taskinfo?taskID=1693312 on the wider set of cases.

I confirm the libicudata.so improvement; it also changes the report on other libraries that we've determined do not contain code.

I'm not sure about the wording of that

   stack-prot test because built from assembler sources

for the iputils case. This sounds like annocheck is very sure that this came from assembler when in fact it was not able to detect where the code came from. Shouldn't the message be much more along the lines of "we have no idea"?

We've already updated out CI to install all debuginfos, so we are covered on that requirement.

Thanks for the changes. I'm afraid I'm not able to provide any insight about the actual detection of the individual cases -- I can just provide pointers to binaries that we've seen as problematic / weird over the time.

(In reply to Jan Pazdziora from comment #28)
Hi Jan,

> I'm not sure about the wording of that
> 
>    stack-prot test because built from assembler sources
> 
> for the iputils case. This sounds like annocheck is very sure that this came
> from assembler when in fact it was not able to detect where the code came
> from. Shouldn't the message be much more along the lines of "we have no
> idea"?

Ah, but in this case there actually is an annobin note in the file which says 
that it was built from assembler!  The note is a fake - it was created by compiling
with the "-Wa,--generate-missing-build-notes" option - which is the method used
when a package does not want annobin notes, but does not want annocheck to
complain about missing notes.  Still annocheck has to believe it.

The iputils-20210202-8.el9 binaries are better - they do actually have real
annobin notes.  (I forgot that they are now held in the debuginfo files).

I have tweaked the annocheck sources again though, so that now the
message reads:

  Hardened: arping: MAYB: test: stack-prot because built from assembler sources 
  Hardened: arping: WARN: If assembler source code is used it may need updating to support stack protection
  Hardened: arping: WARN:  and it definitely needs updating to add a note about stack protection.
  Hardened: arping: WARN: If high level language source code is used then it needs annotation with annobin

Does this feel better to you ?

Cheers
  Nick

Yes, that sounds fair.

Fixed in gcc-toolset-12-annobin-10.97-1.el9

Hello Nick, we have filed this bugzilla (switched over) against annobin because the tool we use in our testing is annobin-annocheck. The latest build in brew is annobin-10.96-1.el9. You list Fixed-in-Version as gcc-toolset-12-annobin-10.98-1.el9. Will you make similar build annobin-10.98? What is the relationship of the annobin component and gcc-toolset-12-annobin?

Hi Jan,

  This BZ has been filed against the sub-component "gcc-toolset-12" so that is why I set the fixed in field to gcc-toolset-12-annobin-10.98.

  There should have been an systemOS annobin-10.97.el9 as well, but I forgot a step in the process - running a CentOS build once my merge had completed.  I have now completed that step and the RHEL build is complete:

    https://brewweb.engineering.redhat.com/brew/buildinfo?buildID=2310116

  It still has to go through gating of course, but it should be in the buildroot soon.

  The only difference between the 10.97 annobin release and the 10.98 release is that the 10.98 has a fix for the gcc plugin to allow it to be built with gcc-13.  Since this is not in the RHEL-9 buildroot, that fix is not needed.

Cheers
  Nick

Thanks for the explanation. I tested annobin-annocheck-10.97-1.el9 and did not see any regressions against 10.96.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (gcc-toolset-12-annobin bug fix and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2023:2297