#19 and #1501 ElfExporter and PeExporter

[astrelsky]

Resolves #19 and resolves #1501.

@fedqx I've attached a ghidra script that implements the fix to this pr so you can use it until this makes it into a release.

FixedBinaryExporter.java.txt

[ghost]

Tested on Ghidra 9.1.1, Ubuntu 18.04, OpenJDK version "11.0.6" 2020-01-14, and I can confirm that it successfully fixed the issue #19
Edit: Tested on a C, Crackme Program

[ryanmkurtz]

This will work for some loaders but not all, so I don't think it's a good idea to change the behavior of the BinaryExporter. For example, take a look at the OmfLoader:129. Also take a look at ElfProgramBuilder:1632. Also, this totally changes the behavior of BinaryExporter. What if someone adds a new block? The old version would have exported the block but the new version would not. Some people may rely on this behavior.

[astrelsky]

This will work for some loaders but not all, so I don't think it's a good idea to change the behavior of the BinaryExporter. For example, take a look at the OmfLoader:129. Also take a look at ElfProgramBuilder:1632. Also, this totally changes the behavior of BinaryExporter. What if someone adds a new block? The old version would have exported the block but the new version would not. Some people may rely on this behavior.

I didn't realize that Memory.getAllFileBytes only returned the original file bytes objects. I also tested creating another memoryblock and it is true that it does not get exported. However, where would such a block go in the file?

I believe ElfProgramBuilder.createExternalFunctionLinkage is making a change at the address of a relocation. I tested this with a binary that dynamically linked to libstdc++ and had external function linkage and the exported program ran without an issue with and without modifications in ghidra. Without making any modifications the exported file was actually identical to the original.

As for the OmfLoader, since the file bytes are still being created. As long as the original file bytes exist the changes should get tracked by the MemoryDB. From what I see in the loader the only written bytes are relocations so those changes would also get reversed.

Worst case scenerio I'll just create an ElfExporter and PeExporter which does this.

[dev747368]

@astrelsky please turn off the automatic "final"izing of variables.

[astrelsky]

@astrelsky please turn off the automatic "final"izing of variables.

It's actually not automatic I did it manually. I'm under the impression that if I don't intend to change a variable I should make it final. Is this not the case? The reason memory and program got set to final is because I actually just rewrote the export method from the DomainObject check down.

I was taught Java by these folks here so I do appreciate small things like this before it turns into a bad habit.

[dev747368]

Ah. I thought it was an editor setting you were using.

Our typical guidelines are to typically use final on class member vars when needed (ie. you are making an immutable class), or static final constants. Usually not on method params or local variables, as it just adds to visual noise. Java itself has gone away from excessive final usage by adding compiler support to figure out if a variable is effectively final when you are accessing them in a lamba or inner class.

As far as the filebytes stuff, my recommendation would be to front the FileBytes's getModifiedBytes() and getOriginalBytes() as InputStreams, and then just delegate the copying to one of many stream copy routines... ie. FileUtilities.copyStreamToStream().

[astrelsky]

Ah. I thought it was an editor setting you were using.

Our typical guidelines are to typically use final on class member vars when needed (ie. you are making an immutable class), or static final constants. Usually not on method params or local variables, as it just adds to visual noise. Java itself has gone away from excessive final usage by adding compiler support to figure out if a variable is effectively final when you are accessing them in a lambda or inner class.

As far as the filebytes stuff, my recommendation would be to front the FileBytes's getModifiedBytes() and getOriginalBytes() as InputStreams, and then just delegate the copying to one of many stream copy routines... ie. FileUtilities.copyStreamToStream().

I see, I should probably go back in some of my code and remove the over use of final then.
I think I've done what you meant by fronting the methods as InputStreams. I'm sure there is a more appropriate place for the private class, especially since it's duplicated, but that's just what I've done for now.

I've restore the BinaryExporter changes and instead created ElfExporter and PeExporter. The BinaryExporter would likely be more for raw binaries. ElfExporter and PeExporter are currently identical but it would be cool to be able to extract the ghidra applied labels and such to the symbol table in the future.

Also it appears the formatting on the stream got obliterated. Is there any way to avoid that?

[astrelsky]

@dev747368 how would you feel about an "AbstractExecutableExporter" that would do the exporting and then in the future the elf and pe variants could them just override its export method, invoke super.export and do things like add in ghidra applied symbols and stuff?

Also, is it safe to assume that the first entry in the list of FileBytes is the currentProgram?

[dev747368]

In general collecting common features into an Abstract base class tends to be good. I haven't dived into the exporter stuff to offer an informed opinion about it though. Same answer about the first entry of FileBytes... I don't know.

Your FileBytes Inputstream needs to use a long for its position instead of an int. And would be good to not duplicate the class twice.

[ghidra1]

I will try to spend some time reviewing this.

[Still34]

Any updates?

[ghost]

@Still34 Well, I have been using @astrelsky 's script and it seems to work fine with C applications. And I have been using it since then. I am not sure when (or even if) this makes into next version but, I am fine with this solution.

[astrelsky]

This will likely take a while to be reviewed given the current situation. As @fedqx stated you could use the script in the meantime. I also hope to implement the symbol exporting sometime after the end of the semester but I make no promises.

[drummachineshavenosoul]

@astrelsky would you be able to write a mach-o exporter? The other two work brilliantly 👏🏻

[astrelsky]

@astrelsky would you be able to write a mach-o exporter? The other two work brilliantly 👏🏻

Try creating an MachOExporter the same way ElfExporter and PeExporter were made it might work. If it doesn't then I'll need to look into the mach-o format a bit. This was intended as a starting point and the abstract base was done to handle differences that would arise later for things such as exporting symbols so the program can be run in a debugger. I don't have any mach-o binaries to do any testing with but if it works I see no issue with adding the class to the pull request.

[drummachineshavenosoul]

@astrelsky after looking at the Exporters I realised that if I deleted the '.' from the end of the outputted file using ElfExporter and chmod the file it worked as a Mach-O file. So maybe that just needs to be added to a 'MachoExporter' but I am completely new to Java so I don't know whether this piece of bash can be used/converted.

[astrelsky]

@astrelsky after looking at the Exporters I realised that if I deleted the '.' from the end of the outputted file using ElfExporter and chmod the file it worked as a Mach-O file. So maybe that just needs to be added to a 'MachoExporter' but I am completely new to Java so I don't know whether this piece of bash can be used/converted.

Ok I'll see about adding it then. I wonder if I can get it to only show up if loadable by a set loader. ie ElfLoader, MachoLoader, etc.

[ryanmkurtz]

I think you can overwrite Exporter.canExportDomainObject() to control it showing up in the list.

[astrelsky]

I think you can overwrite Exporter.canExportDomainObject() to control it showing up in the list.

I took a look at that last night. It takes a Class<? extends DomainObject> though (going off the top of my head.) That would mean it would just be ProgramDB.class. I would actually need an instance of the domain object to check via the Loader.

[ryanmkurtz]

Ah yes, you are right. I guess the best you could do is have the export method fail. Of course, new functionality could be added to the Exporter interface. I think it would be a very non-invasive thing to add.

[darkworks]

I will try to spend some time reviewing this.

have u reviewed it , its 2021 lol

[aFakeCmd]

2021😀

[ghost]

I think people doesn't use the binary export feature of ghidra as often as me since this issue passed its first anniversary 18 days ago 😃 I hope we can fix this issue before its second anniversary.

[eLeCtrOssSnake]

I do use binary export, and it really offends that ghidra can't do such. I am really looking forward to seeing pathing finally work!

[ryanmkurtz]

@astrelsky, would you mind rebasing this? I am going to begin reviewing and testing it for its hopeful inclusion in the next release.

[astrelsky]

@astrelsky, would you mind rebasing this? I am going to begin reviewing and testing it for its hopeful inclusion in the next release.

Will do tonight.

[astrelsky]

@ryanmkurtz you could just cherry pick the commit so you don't have to wait.

[ryanmkurtz]

I try to avoid doing that because it breaks the PR's link to the original commit, preventing the purple merged state from ever appearing. Having said that, we do it all the time for less active users so I can definitely do it here too. I'll begin testing it with the cherry picked commit. If you feel like rebasing tonight, i'll continue the work off of that. Otherwise I'll just use the cherry picked commit to move it forward.

[ryanmkurtz]

@astrelsky There are some things that will need to be changed. Did you want the opportunity to make the changes or would you prefer if I just make them?

[astrelsky]

@astrelsky There are some things that will need to be changed. Did you want the opportunity to make the changes or would you prefer if I just make them?

It is fine if you make them as my resources are a bit constrained at the moment.

[dragonmacher]

It is fine if you make them as my resources are a bit constrained at the moment.

Sounds like you need a faster potato.

[astrelsky]

It is fine if you make them as my resources are a bit constrained at the moment.

Sounds like you need a faster potato.

Well I definitely do but I'm currently staying in a hotel. I may be a bit over paranoid but I haven't even turned on my potato since I got here because I don't trust the unsecured wifi.

[ryanmkurtz]

@ryanmkurtz you could just cherry pick the commit so you don't have to wait.

I'm committed to moving forward with the cherry pick now, so no need to rebase.

[astrelsky]

@ryanmkurtz you could just cherry pick the commit so you don't have to wait.

I'm committed to moving forward with the cherry pick now, so no need to rebase.

Ok, I have no problem with that.

[eLeCtrOssSnake]

Is there any commit available to be pulled from to test the changes?

[eLeCtrOssSnake]

I still believe there's an issue with relocating variables. Because the exporting script can't properly reference variables written with "Patch Instruction".

[astrelsky]

I still believe there's an issue with relocating variables. Because the exporting script can't properly reference variables written with "Patch Instruction".

Do you mean when you have patched an instruction where a relocation is present? If so this is an interesting point.

[ryanmkurtz]

The documentation for this feature will look something like this:

PE Exporter:

Writes a PE program that was imported with the PE loader back to its original file layout. 
Any file-backed bytes that were modified by the user in the program database will be reflected 
in the new file.

Note: Writing back a modified Memory Map is not supported.

Note: Relocation bytes are always restored to their original values, even if the user modifies them.

[eLeCtrOssSnake]

I still believe there's an issue with relocating variables. Because the exporting script can't properly reference variables written with "Patch Instruction".

Do you mean when you have patched an instruction where a relocation is present? If so this is an interesting point.

I believe so yeah

[astrelsky]

I still believe there's an issue with relocating variables. Because the exporting script can't properly reference variables written with "Patch Instruction".

Do you mean when you have patched an instruction where a relocation is present? If so this is an interesting point.

I believe so yeah

Unfortunately with the current API this is impossible to handle. This is why it was explicitly noted in the documentation for the exporters. In order to be able to support it there would need to be a plethora of changes to the way relocations are handled and possibly some changes to sleigh.

I would actually welcome a complete overhaul of how relocations are handled. It is not convenient at all to implement handling of new relocations.

[eLeCtrOssSnake]

As much I was waiting for exporting to be implemented, I can't seem to build Ghidra from the current upstream.
This is what I get: https://pastebin.com/X9qZ3v1X
I was following the steps from the building guide. I am not very sure if this is relevant, but I have no experience with java projects.
But I really want to try out the exporter feature.
* I am using gradle 5.0 and jdk-11