Page MenuHome GnuPG

libgcrypt: More robust/portable integrity check
Closed, ResolvedPublic

Description

For FIPS, when --enable-hmac-binary-check, we (somehow experimentally) use a section of ELF to store the hmac hash.

It works for major use cases, but we see problems:

  • using ld.gold
  • using clang

Besides, it would be good if it allows change of the binary by strip or similar operations.

Details

Version
1.10

Event Timeline

gniibe triaged this task as High priority.Feb 15 2022, 3:38 AM
gniibe created this task.
gniibe updated the task description. (Show Details)

I am going to apply https://gitlab.com/redhat-crypto/libgcrypt/libgcrypt-mirror/-/commit/64ccc25c4b4a2c8c4e13e7e37ff1c8c60a3d8401
And consider adding the code to limit hashing content (from start of the file to end of data section).

Thanks! Maybe it would be simpler to use dl_iterate_phdr(3) for this. I wasn't aware of the function, but a colleague just implemented a proof-of-concept of what you're proposing in https://gitlab.com/dueno/integrity-notes.

Folks, you are opening a can of worms. The only secure why to sign a file is to have a detached signature. That is often non-practical and thus putting the signature/MAC at one certain position and exempt just this one position from hashing is the next best alternative. Any more complicated rules will inevitably introduce security flaws. If a binary is stripped, it is a different binary than a non-stripped one, if it is linked with another linker, it is a different one. And that binary will even be able to figure this out and change behavior. Please keep it simple.

Thank you for your suggestions, @werner.
I agree that we should not put much effort to develop our own methodology here; Too much effort may introduce possibility of unmaintainable code, which should be avoided for the particular purpose of "integrity".

Eventually, (in future), modern approach like https://gitlab.com/dueno/integrity-notes will become common practice, say, supported by an implementation of C library, then, we just can use it. I hope.

For libgcrypt, it's a narrow path, as its intended use case are wider. While the support of integrity check for FIPS is only for GNU system (for now), we would like to avoid introducing much of GNU system specific code into libgcrypt.

  • Use of non-portable dladdr1 and struct link_map only for access to a particular member would be OK.
  • Use of types in elf.h would be OK.

(For someone, libgcrypt can be ported to non-GNU system with ELF.)

Then, now, if we switch to...

  • Use of dl_iterate_phdr and struct dl_phdr_info

I'm afraid it's too much.

Besides, I learned this is useful:
https://gitlab.com/dueno/integrity-notes/-/blob/master/integrity.c#L114

Another point: For libgcrypt, it woud not OK: the way generate-integrity-note.c accessing target shared library by dlopen. Unfortunately, libgcrypt.so for FIPS has constructor to check its integrity. Even if the computation of hmac itself may work (when the hmac computation doesn't use libgcrypt), it's somehow insane to run code of libgcrypt which fails (as a side effect of loading the shared library) to compute its hmac hash.

Pushed the change which fixes the build with ld.gold.
rC9dcf9305962b: fips: Integrity check improvement, with only loadable segments.

I'm open to the idea of use of .note.integrity section, as I think that it may fix the build with clang.
The point where I don't understand:

  • Is the note section need to be loaded on memory?
    • the statement .section ".note.integrity", "a"
  • If it's OK not to be loaded, we can just --add-section to libgcrypt.so, perhaps

I pushed the change: rCa340e9803882: fips: More portable integrity check.
It uses .note.fdo.integrity section, not loaded onto memory.
It simplifies the logic, and switches to dladdr (from dladdr1).

I located the cause:

../../src/gen-note-integrity.sh: line 78: cmp: command not found

Let me consider, how I can survive in the environment with no cmp.

Ah, right, I can get that added to the containers tomorrow.

I simplified the script not to use cmp: rC3c8b6c4a9cad: fips: Fix gen-note-integrity.sh script not to use cmp utility.
And I clarified the semantics of the integrity check.

  • We add hmac value bytes onto the shared library file itself.
    • by .note.fdo.integrity section
    • It is not to be loaded onto memory when running
    • only in the file, as a noload section
  • This method is good, because it is only the shared library file, the single file, no more
  • On the other hand, adding the hmac bytes modifies the shared library file
    • We learned that objcopy --update-section may change other parts in the file (not only the target section).
  • For the computation of hmac value, it should exclude the part of hmac bytes in the share library file
    • it is easier when it's done by a noload section, then
    • target contents to be hashed are simply:
      • all segments of the shared library
      • ELF header information for program headers (sans section information)
      • because section headers and section information is for linking-time, not runtime

and pushed the implementation change as: rC052c5ef4cea5: fips: Clarify what to be hashed for the integrity check.

Note that the commit doesn't change the hmac resulted, in the current layout of ELF file.

I have one follow-up is that the readelf chokes on the integrity note for some reason:

$ readelf -n /usr/lib64/libgcrypt.so.20.4.1
Displaying notes found in: .note.fdo.integrity
  Owner                Data size 	Description
  FDO                  0x00000020	Unknown note type: (0x8e2afeca)

I assume this is just because the readelf does not know this type. I see this type was initially proposed by Daiki, but I did not find any other sources for this magic number so before filling bugs for readelf, do we have some doc why the 0xcafe2a8e is used?

I just copied the value of 0xcafe2a8e and the name .note.fdo.integrity from Daiki's implementation. No other reason.

I was pointed by Daiki to the following patch in Fedora binutils, which allows listing the fdo packaging metadata, but it does not list any other unknown objects and unfortunately fails hard:

https://src.fedoraproject.org/rpms/binutils/blob/rawhide/f/binutils-readelf-recognize-FDO-Packaging-Metadata-ELF-note.patch#_42

So for now, we are probably ok, but as we will get to the point where we would have at least two libraries using this integrity note, we will probably want to have readelf updated for this, but this is probably on us.