Re: Request for review/upload of regripper 3.0-1
Dear Raphaël,
thank you for your thorough review, your questions and the hints.
On 08.06.21 15:31, Raphael Hertzog wrote:
Hello,
I have looked at regripper. Here are my comments/questions:
In the README I see this:
The following Perl module files have been modified, and the modified versions are
provided as part of this repo:
C:\Perl\site\lib\Parse\Win32Registry\WinNT\File.pm
C:\Perl\site\lib\Parse\Win32Registry\WinNT\Base.pm
C:\Perl\site\lib\Parse\Win32Registry\WinNT\Key.pm
If you're using the Windows EXE versions of the tools, this is irrelevant, as the
modified files are "compiled" into the EXE. However, if you're installing on Linux,
copy the files from the repo to the appropriate locations in your installation.
Have you looked at what kind of modifications have been made and whether they
are still relevant ?
The changes to Base.pm alter the conversion of Windows filetime to
epoch-timestamps inside the subroutine `unpack_windows_time` [0]. The
changes to File.pm extend Parse/Win32Registry/WinNT/File.pm by adding
another timestamp parsing routing [1] and exposing additional member
variables with getter-subroutines [2]. The changes to Key.pm extend
Parse/Win32Registry/WinNT/Key.pm by a field containing `access_bits` and
a subroutine to retrieve the lenght of the largest subkey-name.
Those changes seem to be relevant but not "mission-critical", since only
limited part of regripper's functionality depends on those modifications.
(In my workflow those patches were actually not relevant, so that I did
not notice those issues in all the time I have been using the tool.)
Because right now, with the way how you install these files, I don't think
that they are used at all:
- you modify the perl search path to add /usr/lib/regripper at the end of the
search path (with the quite ugly "push @INC, $str") so it will not take
precedence over the system wide module
- you are installing them as /usr/lib/regripper/{File,Base,Key}.pm and not
as /usr/lib/regripper/Parse/Win32Registry/WinNT/{File,Base,Key}.pm
I don't really like the changes in 10_modify-paths-in-srcs.patch (but I
also don't like the initial upstream code that you had to modify):
-#push(@INC,$str);
+push(@INC,$str);
Is this really needed given that my question above ? Also I think it adds
"/usr/bin" to @INC and not /usr/lib/regripper as you hoped.
You are absolutely right in all regards. I have to admit, that I adapted
the changes without the needed critical look, since the code references
were attributed to a renowned person in the forensics community.
-#my $plugindir = $str."plugins/";
-#my $plugindir = File::Spec->catfile("plugins");
+my $plugindir = $str."/usr/lib/regripper/plugins/";
+my $plugindir = File::Spec->catfile("/usr/lib/regripper/plugins");
You don't need to define the variable twice.
Corrected.
In the interest of having a single codebase that works on Linux and
Windows, I think we should aim to patch the code so that it finds the
plugins directory next to where the code lives. Given that $0 returns
/usr/bin/regripper or possibly (./rip.pl if called manually in
/usr/lib/regripper), you would likely have to use Cwd::realpath($path)
to find out the real location of the code.
If you don't want to pursue the upstreaming of this patch, then I would
simply use "use lib '/usr/lib/regripper'" instead of your push @INC
and unconditional setting of $plugindir like you did.
Thanks for these hints. I changed the installation path of these files
and modified my patch `10_modify-paths-in-srcs.patch` according to your
requests [3]. I decided to clean up the code concerned with the
construction of the plugin path and went with `use lib
/usr/lib/regripper` (instead of `unshift @INC, $scriptdir;`) I verified,
that the patched Win32Registry-files have precedence over the system
wide modules, which are installed as a dependency with apt.
Furtheron, I submitted the a/m patch -- without the
distribution-specific `use lib ...`-statement -- to upstream, since I
think, that those changes clarify the code for the creation of the
plugin-directory's path [4].
Looking at the autopkgtest, I have a problem with listplugins as this test
will automatically fail for any new plugin added. I'd rather have tests
that do not fail just because we have a new plugin... the janitor bot can
prepare new upstream snapshots and we don't want the bot to believe that
his snapshot is broken just because it contains a new plugin. I think it's
OK to fail if a plugin disappears.
Hint: you could use "combine $TARGET not $ACTUAL" (moreutils package) to
see if some lines disappeared from the CSV output.
Corrected. I used `$(combine ${ACTUAL} not ${TARGET} | wc -l)` to check,
if there are missing plugins.
The test passes now as requested, even if there were added new plugins.
Thanks for the tip on `combine`!
Small nitpicks:
* in 40_correct_mountdev2_plugin.patch and
50_correct_scanbutton_plugin.patch you have "Origin" fields that are not
really useful. You are the author of the patch and thus you are the origin,
not the upstream author (even if he merged your patches).
Corrected -- removed "Origin"-fields.
* in 20_correct-encoding.patch you are missing a leading space at the
start of the long description of the patch (before "upstream stores")
Corrected -- added a space in front of the long description.
Thank you again for reviewing the package and providing detailed
feedback as well as hints on the solution of the issues.
I already submitted a merge request containing my changes [5]. I hope, I
have addressed all issues in an adequate manner.
Please have indulgence for my blunders as this is my very first
packaging attempt.
Best regards
Jan
---
[0]
https://github.com/keydet89/RegRipper3.0/blob/c61d937b392c73f7719357bd39b7fefe1e8b48f2/Base.pm#L168
[1]
https://github.com/keydet89/RegRipper3.0/blob/c61d937b392c73f7719357bd39b7fefe1e8b48f2/File.pm#L57
[2]
https://github.com/keydet89/RegRipper3.0/blob/c61d937b392c73f7719357bd39b7fefe1e8b48f2/File.pm#L169
[3]
https://salsa.debian.org/jgru/regripper/-/blob/add-changes-for-2nd-review/debian/patches/10_modify-paths-in-srcs.patch
[4] https://github.com/keydet89/RegRipper3.0/pull/34
[5] https://salsa.debian.org/pkg-security-team/regripper/-/merge_requests/1
Reply to: