Using unsafe in control flow obfuscation

[pagran]

Using unsafe when obfuscating will allow to do some interesting things, such as:

1) Fix lazy map iteration problem (idea from here)
2) Add data obfuscation. For example, automatic prevention of string disclosure via GC. This can be implemented via unsafe.StringData method and force zeroing of string data.

In addition to "unsafe" is unsafe, i see a potential problem in the future with exporting code (#369), because using unsafe can break compatibility between versions of go.

What's your opinion?

@mvdan @lu4p

[pagran]

Unexpectedly for me, reflect has an iterator for maps - https://pkg.go.dev/reflect#Value.MapRange

[mvdan]

Why does the SSA->AST conversion break map ranges? At least https://github.com/burrowers/garble/blob/master/docs/CONTROLFLOW.md#caveats and the linked code don't give a conclusive answer.

Note that using https://pkg.go.dev/reflect#Value.MapRange won't be a great solution in general. Converting a Go value to a reflect.Value incurs a small amount of CPU work (and allocations) which would likely harm performance.

As for "hiding" data in memory, from either the GC or inspection by any third parties: I think you want golang/go#21865. Note that using it for absolutely every string or bytes value will most likely have too high of an overhead, so I don't think we should do it in general. I think the code authors should use it for any sensitive or secret information they wish to briefly keep in memory.

Those two particular cases aside, I'd really want to stay away from unsafe. Using it is likely to increase the chances of subtle bugs and maintenance nightmares going forward :) garble already relies on quite a few internal and undocumented behaviors from the toolchain, so I wouldn't want to also start messing with the runtime if we can avoid it.

[pagran]

Main reason of the lazy map iteration broken is that there is no for loop at the ssa level. A map iteration in ssa is a call of Next and If + Jump, and there is no analog in go (except reflect equivalent). Theoretically, it is possible to analyze ssa code like a decompiler and reconstruct iteration, but it is too complicated for simple converting.

A reflection looks like a good compromise to me, because the loss on very large maps will be minimal compared to a temporary array with all keys.

About unsafe i agree with your arguments, this functionality is too unstable+unsafe for master branch

[mvdan]

Theoretically, it is possible to analyze ssa code like a decompiler and reconstruct iteration, but it is too complicated for simple converting.

Honestly I would say that is the right solution. It's going to be a bit of work to implement it for sure, but the alternatives are all considerably worse in the long run.

Regarding reflect being a good compromise - note that reflect will incur a fixed amount of overhead for every range, so ranging over a tiny map might get multiple times slower. This kind of added cost could really slow down Go programs, for example net/http which uses maps for headers.

[pagran]

for example net/http which uses maps for headers.

Sounds reasonable, until it's fixed enable globally is unsafe

[pagran]

@mvdan benchmark results unexpected (y: time, x: map size)

full iterate: http://benchgraph.codingberg.com/1le
half iterate: http://benchgraph.codingberg.com/1lf

*Keys = current implementation

[mvdan]

I did mention that reflection has an overhead :) That is still a bit higher than I expected, but it's still not surprising.