Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Line numbers in the output doesn't match with the Line numbers in the source code #37

Closed
abhi06991 opened this issue Feb 16, 2024 · 16 comments
Assignees

Comments

@abhi06991
Copy link

Hi,

I was testing out this tool and works really well, its able to find the issues if the patterns from the rules config file are matched, but the line numbers that comes with the output don't match with the line number from the source code. This could be a bit frustrating for the developers/security analysts while going through the results. I am attaching the sample file that was tested and also the results that the tool gave as a zip file

Any fix regarding this would be great!

Thanks
zarn files.zip

@htrgouvea
Copy link
Owner

Hi @abhi06991,

Sorry for this error. I made a change and I believe that now the numbers will be closer to correct. Can you check and tell me if it has improved?

Thank you!

@abhi06991
Copy link
Author

Hi @htrgouvea ,

Thanks for working on this. But, the issue still exists, couldn't see much improvement.

Some results from the latest scan -
[vuln] - FILE:/home/runner/work/ds-experiments-devops-devops-codeqltest/ds-experiments-devops-devops-codeqltest/vulnerable.pl Potential: LDAP Injection. Line: 66:23 -> The actual snippet is in the range of line numbers, 33-35
[vuln] - FILE:/home/runner/work/ds-experiments-devops-devops-codeqltest/ds-experiments-devops-devops-codeqltest/vulnerable.pl Potential: SQL Injection. Line: 87:17 -> The sample code for sql injection is between line numbers 131-168

Also, just for your information, I am not using the github action for Zarn from the market place. I am manually cloning the repo and using the tool, like this (hopefully I am using the tool the right way)-

 - name: Downloading Zarn repository from Github
    run: |
      cd ..
      git clone https://github.com/htrgouvea/zarn && cd zarn
      sudo apt-get update  
      sudo apt-get -y install cpanminus
      sudo cpanm --installdeps .

@htrgouvea htrgouvea reopened this Feb 21, 2024
@htrgouvea
Copy link
Owner

Hi @abhi06991,
ZARN has a functionality to ignore blank lines and lines in comments, it removes them during analysis.

zarn/lib/Zarn/AST.pm

Lines 23 to 24 in 4cf284d

$document -> prune("PPI::Token::Pod");
$document -> prune("PPI::Token::Comment");

I believe that now this is the only reason that the lines in the findings are wrong. I can remove this functionality. We're going to lose a little bit of performance but I think it's okay.

@htrgouvea
Copy link
Owner

I commented on these resources and uploaded new code to the develop branch: https://github.com/htrgouvea/zarn/tree/develop, can you test again? If it improves, I'll put the code in the main branch.

To do this it would be something like:

git clone https://github.com/htrgouvea/zarn && cd zarn
git checkout develop
sudo apt-get -y install cpanminus
sudo cpanm --installdeps .

Thanks.

@abhi06991
Copy link
Author

Just tested this out, I guess something got messed up. Previously Zarn was showing 17 vulnerabilities, now its giving 50 vulnerabilities. Many vulnerabilities are getting duplicated with different random line numbers.

Attached the output result with the comment
zarn-output.txt

@htrgouvea
Copy link
Owner

@abhi06991 Could I take a look at your rules file too? Since they are custom rules, this would help me understand better.

@abhi06991
Copy link
Author

sure, pls check this -
custom-rules-config.txt

@m-1-k-3
Copy link

m-1-k-3 commented Feb 21, 2024

Hi @htrgouvea, I had the same issue today and the modifications in the development branch fixed it.

@htrgouvea
Copy link
Owner

Hi folks, I deployed the develop code to the main branch. @abhi06991 I believe your problem now is with the custom rules, I'm investigating more this part.

@htrgouvea
Copy link
Owner

I'll implement a different response, with more details. The lines still won't be 100% accurate, but I believe it will improve. I'll let you know soon.

@m-1-k-3
Copy link

m-1-k-3 commented Feb 21, 2024

thx for your effort! Great work!

@m-1-k-3
Copy link

m-1-k-3 commented Feb 23, 2024

Something not working with the main branch

1st the develpment branch from 2 days ago (looking good):

┌──(m1k3㉿emba)-[~/github-repos/zarn]
└─$ perl zarn.pl -r ./rules/default.yml --source /home/m1k3/firmware-stuff/Firmware_images/testimages/web/htdocs_pulse_cgi/dana/home/launch.pl
[vuln] - FILE:/home/m1k3/firmware-stuff/Firmware_images/testimages/web/htdocs_pulse_cgi/dana/home/launch.pl      Potential: Code Injection.      Line: 488:14
[vuln] - FILE:/home/m1k3/firmware-stuff/Firmware_images/testimages/web/htdocs_pulse_cgi/dana/home/launch.pl      Potential: Code Injection.      Line: 535:22
[vuln] - FILE:/home/m1k3/firmware-stuff/Firmware_images/testimages/web/htdocs_pulse_cgi/dana/home/launch.pl      Potential: Code Injection.      Line: 547:30
[vuln] - FILE:/home/m1k3/firmware-stuff/Firmware_images/testimages/web/htdocs_pulse_cgi/dana/home/launch.pl      Potential: Code Injection.      Line: 558:22

2nd the main branch from today (produces wrong output):

┌──(m1k3㉿emba)-[~/github-repos/zarn]
└─$ cd ../zarn_new 
                                                                                                                                                                                                                                            
┌──(m1k3㉿emba)-[~/github-repos/zarn_new]
└─$ perl zarn.pl -r ./rules/default.yml --source /home/m1k3/firmware-stuff/Firmware_images/testimages/web/htdocs_pulse_cgi/dana/home/launch.pl
[vuln] - FILE:/home/m1k3/firmware-stuff/Firmware_images/testimages/web/htdocs_pulse_cgi/dana/home/launch.pl      Potential: Code Injection.      Line: 473:14
[vuln] - FILE:/home/m1k3/firmware-stuff/Firmware_images/testimages/web/htdocs_pulse_cgi/dana/home/launch.pl      Potential: Code Injection.      Line: 519:22
[vuln] - FILE:/home/m1k3/firmware-stuff/Firmware_images/testimages/web/htdocs_pulse_cgi/dana/home/launch.pl      Potential: Code Injection.      Line: 530:30
[vuln] - FILE:/home/m1k3/firmware-stuff/Firmware_images/testimages/web/htdocs_pulse_cgi/dana/home/launch.pl      Potential: Code Injection.      Line: 540:22

@m-1-k-3
Copy link

m-1-k-3 commented Feb 23, 2024

git reset --hard 009331c

brings you to the state with correct line numbers

@htrgouvea
Copy link
Owner

@m-1-k-3 This version of the code was when I disabled the feature that ignores comment lines and blank lines, however leaving the comments lines present, the number of false positives started to increase. So I reactivated the functionality.

@htrgouvea
Copy link
Owner

Hi folks,

The line number of the function that has the vulnerability remains an approximate number. This happens because ZARN removes all comments from the file, this helps to reduce the number of false positives.

To help, ZARN's output now also has the line number of the variable that is possibly controlled by the attacker, the payload delivery point. This number is an exact number.

I believe this can help. Thank you all.

@m-1-k-3
Copy link

m-1-k-3 commented Mar 26, 2024

I understand your point but the temp fixed version shows (as far as I have seen) only FP in comments and this should be quite easy to detect and remove from the results. Wouldn't this work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants