ROX-8450: Extract elf metadata with matcher #535

c-du · 2021-11-19T18:13:22Z

Extract metadata from elf executable
Create matcher and populate elf metadata to file data.

RTann · 2021-11-19T18:43:10Z

pkg/elf/elf.go

+
+// MetaData contains the exacted metadata from ELF file
+type MetaData struct {
+	SoNames           []string


Can you add a comment specifying these are .so file names?

Added comment

Maybe just SharedObjects would be a more descriptive name

I will keep it as soname because that is what it is. Shared objects have more meaning than soname.

RTann · 2021-11-19T18:47:12Z

pkg/elf/elf.go

+func IsElfExecutable(r io.ReaderAt) bool {
+	var elfBytes = make([]byte, len(elf.ELFMAG))
+
+	_, err := r.ReadAt(elfBytes, 0)


let's actually remove this check and just leave it up to elf.NewFile to check the magic number for us

yes, should work.

pkg/elf/elf.go

RTann · 2021-11-19T18:49:49Z

pkg/elf/elf.go

+)
+
+// MetaData contains the exacted metadata from ELF file
+type MetaData struct {


nit: metadata is one word, so Metadata might be better

RTann · 2021-11-19T18:50:09Z

pkg/elf/elf.go

+}
+
+// GetElfMetadataData extracts and returns ELF metadata
+func GetElfMetadataData(r io.ReaderAt) (*MetaData, error) {


nit: GetElfMetadata

RTann · 2021-11-19T18:58:18Z

pkg/matcher/matcher.go

@@ -54,6 +55,7 @@ func NewWhiteoutMatcher() Matcher {
 type executableMatcher struct{}

 func (e *executableMatcher) Match(_ string, fi os.FileInfo, _ io.ReaderAt) (matches bool, extract bool) {
+	// Something


RTann · 2021-11-19T18:58:34Z

pkg/matcher/matcher.go

+	return elf.IsElfExecutable(contents), false
+}
+
+// NewElfMatcher returns a matcher that matches if and only if any of the passed submatchers does.


copy paste?

Yeah, Fixed.

RTann · 2021-11-19T18:58:47Z

pkg/tarutil/tarutil.go

@@ -64,6 +66,12 @@ type FileData struct {
 	Contents []byte
 	// Executable indicates if the file is executable.
 	Executable bool
+	// ElfMetaData contains the dynamic library dependency metadata if the file is in ELF format.
+	ElfMetaData *elf.MetaData


nit: ElfMetadata

RTann · 2021-11-19T18:59:02Z

pkg/tarutil/tarutil.go

+	ElfMetaData *elf.MetaData
+}
+
+func (f *FileData) String() string {


RTann · 2021-11-19T19:02:30Z

pkg/tarutil/tarutil.go

@@ -77,6 +85,7 @@ func ExtractFiles(r io.Reader, filenameMatcher matcher.Matcher) (FilesMap, error
 	// executableMatcher indicates if the given file is executable
 	// for the FileData struct.
 	executableMatcher := matcher.NewExecutableMatcher()
+	elfMatcher := matcher.NewElfMatcher()


I noticed we didn't add the ELF matcher to the filenameMatcher (see singletons/requiredfilenames/matcher.go). Is it not necessary? I guess ELF files are all executable, so there may not be a need. I just want to confirm that the current matchers are sufficient enough to capture all ELF files

Oh, I forgot not pull it from the draft because it is in dpkg package. Added it back. It is needed because most libraries do not have execution mode.

RTann

A few comments. Also, this adds functionality which will be used later, but is not used now. I'm wondering if we should add another feature flag to put this behind? Maybe that's overkill

RTann · 2021-11-23T17:15:05Z

pkg/elf/elf.go

+	}
+
+	// Exclude core and other unknown elf file.
+	return set.NewFrozenIntSet(int(elf.ET_EXEC), int(elf.ET_DYN)).Contains(int(elfFile.Type))


nit: maybe make this a global elfTypeAllowlist = set.NewFrozenIntSet(...)

RTann · 2021-11-23T17:16:40Z

pkg/elf/elf.go

+	if err != nil {
+		return nil, err
+	}
+	soName, err := elfFile.DynString(elf.DT_SONAME)


nit: I think it's usually soname? Not 100% sure because I just looked it up right now. Not a big deal, though

RTann · 2021-11-23T17:18:49Z

pkg/tarutil/tarutil.go

+			isElf, _ := elfMatcher.Match(filename, hdr.FileInfo(), contents)
+			if isElf {
+				if elfMetadata, err := elf.GetElfMetadata(contents); err != nil {
+					log.Errorf("Failed to get dependencies for %s", filename)


nit: can you specify this is an ELF file? Failed to get dependencies for elf file %s

Added that to inner function and appended the inner error here.

RTann · 2021-11-23T17:19:09Z

pkg/tarutil/tarutil.go

@@ -131,13 +135,21 @@ func ExtractFiles(r io.Reader, filenameMatcher matcher.Matcher) (FilesMap, error

 		// Extract the element
 		if hdr.Typeflag == tar.TypeSymlink || hdr.Typeflag == tar.TypeLink || hdr.Typeflag == tar.TypeReg {
-			executable, _ := executableMatcher.Match(filename, hdr.FileInfo(), contents)
+			fileData := FileData{}


nit: var fileData FileData

RTann · 2021-11-23T17:20:31Z

singletons/requiredfilenames/matcher.go

-	once     sync.Once
+	instance         matcher.Matcher
+	once             sync.Once
+	dynamicLibRegexp = regexp.MustCompile(`(^|/)lib[^/.]*\.so(\.[^/.]+)*$`)


nit: dynamicLibPattern

Also, maybe put a newline before this?

Can you add some tests for this regex that simply take some strings and check that it matches ones we expect to match? This will validate that the regex matches what we expect and also give some examples of what it should match

RTann · 2021-11-23T17:24:52Z

singletons/requiredfilenames/matcher.go

@@ -39,7 +41,9 @@ func SingletonMatcher() matcher.Matcher {
 			// Therefore, this matcher MUST be the last matcher.
 			executableMatcher := matcher.NewExecutableMatcher()

-			allMatchers = append(allMatchers, dpkgFilenamesMatcher, executableMatcher)
+			dynamicLibMatcher := matcher.NewRegexpMatcher(dynamicLibRegexp)


let's put this one above the executableMatcher addition and append it before the executableMatcher. This will be necessary because the executableMatcher does not tell us to extract the file contents, but the RegexpMatcher does, and we will need to extract the so file contents (I believe).

Also, can you bump the allMatchers := make([]matcher.Matcher, 0, 4) to 5?

pkg/elf/elf.go

connorgorman · 2021-11-23T18:42:52Z

pkg/elf/elf.go

+
+// MetaData contains the exacted metadata from ELF file
+type MetaData struct {
+	SoNames           []string


Maybe just SharedObjects would be a more descriptive name

connorgorman · 2021-11-23T18:43:42Z

pkg/elf/elf.go

+	}
+
+	// Exclude core and other unknown elf file.
+	return set.NewFrozenIntSet(int(elf.ET_EXEC), int(elf.ET_DYN)).Contains(int(elfFile.Type))


connorgorman · 2021-11-23T18:44:21Z

pkg/elf/elf.go

+	}
+	soName, err := elfFile.DynString(elf.DT_SONAME)
+	if err != nil {
+		return nil, err


can we wrap these errors with what action failed? I worry the internal message may not be clear

Yes, added that.

connorgorman · 2021-11-23T18:44:24Z

pkg/elf/elf.go

+	}
+	libraries, err := elfFile.ImportedLibraries()
+	if err != nil {
+		return nil, err


can we wrap these errors with what action failed? I worry the internal message may not be clear

connorgorman · 2021-11-23T18:44:52Z

pkg/elf/elf.go

+		return nil, err
+	}
+	return &Metadata{
+		SoNames:           soName,


find it a bit strange we assign a singular variable name to a plural. maybe soNames for the var?

Yes, fixed. elf.DynString sometimes gives me a feeling that this is singular.

connorgorman · 2021-11-23T18:47:26Z

singletons/requiredfilenames/matcher.go

-	once     sync.Once
+	instance         matcher.Matcher
+	once             sync.Once
+	dynamicLibRegexp = regexp.MustCompile(`(^|/)lib[^/.]*\.so(\.[^/.]+)*$`)


Can you add some tests for this regex that simply take some strings and check that it matches ones we expect to match? This will validate that the regex matches what we expect and also give some examples of what it should match

connorgorman

Looks good!

RTann

LGTM just two nits to consider before merging

RTann · 2021-11-23T23:51:41Z

pkg/elf/elf.go

+	ImportedLibraries []string
+}
+
+// OpenIfElfExecutable tests if the data is in ELF format


nit: update the comment to reflect new functionality

RTann · 2021-11-23T23:52:47Z

pkg/elf/elf.go

+)
+
+var (
+	allowedElfTypeList = set.NewFrozenIntSet(int(elf.ET_EXEC), int(elf.ET_DYN))


nit: should we do allowedELFTypeList as well as changing other instances of Elf with ELF?

c-du added 3 commits November 19, 2021 10:05

ROX-8450: Extract elf metadata with matcher

09d3aa5

name changes

5642f77

missing linux binary

7cafcb4

c-du requested review from RTann and connorgorman November 19, 2021 18:34

RTann reviewed Nov 19, 2021

View reviewed changes

resolve review comment

3af3333

c-du requested a review from RTann November 19, 2021 23:43

c-du marked this pull request as ready for review November 20, 2021 01:11

c-du added 2 commits November 19, 2021 18:31

be nice to people putting lib under root

1563312

Merge branch 'master' into cong-avm2-elf

a952021

RTann reviewed Nov 23, 2021

View reviewed changes

pkg/elf/elf.go Show resolved Hide resolved

connorgorman suggested changes Nov 23, 2021

View reviewed changes

Resolve comments

3737024

c-du requested review from RTann and connorgorman November 23, 2021 23:17

connorgorman approved these changes Nov 23, 2021

View reviewed changes

blank

ec7e4b9

RTann approved these changes Nov 23, 2021

View reviewed changes

c-du added 2 commits November 23, 2021 16:58

elf to ELF

504e2ae

Merge branch 'master' into cong-avm2-elf

1304e5c

c-du merged commit 1d4958c into master Nov 24, 2021

c-du deleted the cong-avm2-elf branch November 24, 2021 01:28

ROX-8450: Extract elf metadata with matcher #535

ROX-8450: Extract elf metadata with matcher #535

Uh oh!

Conversation

c-du commented Nov 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RTann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment