Perl - Source code review

bheemamahesh has asked for the wisdom of the Perl Monks concerning the following question:

Replies are listed 'Best First'.
Re: Perl - Source code review by choroba (Cardinal) on Nov 13, 2014 at 08:19 UTC
The project is not small. Do you really think someone will review it for free? The person who wrote the code was probably more skilled in a different programming language (C?). There is no strict. I don't understand some of the idioms used there, e.g.: `while (defined(pop @LogFileList)) {};` [download] Basically, we try to help people who want to learn. Describe what the project does, what you already tried and where you got stuck. Then we can help you. لսႽ† ᥲᥒ⚪⟊Ⴙᘓᖇ Ꮅᘓᖇ⎱ Ⴙᥲ𝇋ƙᘓᖇ	[reply] [d/l]
Re^2: Perl - Source code review by SuicideJunkie (Vicar) on Nov 13, 2014 at 14:38 UTC
The project is not small. Do you really think someone will review it for free? I'm pretty sure the free reviewer is the student Bheemamahesh. As I understand the question, the project is not necessarily intended to accomplish anything... but it is supposed to do nothing or anything in a safe manner. And there is a bug hidden in the code to find. Based on that, I would suggest starting with all the standard advice (strict, warn, 3-arg open, etc, etc) and take close notice of the problems that they are intended to prevent. Make a big list (Variable typos, undefined values, barewords, borked references, changing the r/w type of a filehandle, etc.) With those kinds of problems in mind, run through the code in your head, and imagine/simulate what would happen on hostile input.	[reply]
Re^2: Perl - Source code review by Jenda (Abbot) on Nov 13, 2014 at 10:15 UTC
I assume the curlies are not empty in the original, right? The loop removes the elements from the array one by one starting at the end and stops once there are no more elements to remove. The last element is at the start of the first iteration, before the body of the loop runs and as it is not assigned anywhere it most probably is not processed. If the curlies were empty and the array was not tie()d, then the line would be equivalent to `@LogFileList = ();` The only case in which it would make some sense to use that line with empty curlies would be if the array was tie()d removing elements from the tie()d array had some important side-effects you wanted the side effects to occure starting with the last element Jenda Enoch was right! Enjoy the last years of Rome.	[reply] [d/l]
Re^3: Perl - Source code review by Anonymous Monk on Nov 13, 2014 at 10:24 UTC
I assume the curlies are not empty in the original, right? Wrong :) If the curlies were empty and the array was not tie()d, then the line would be equivalent to @LogFileList = (); Except worse :) `perl -E 'my @ary = (0, 1, undef, 3); while (defined(pop @ary)) {}; say join "\|", @ary'` Which is probably not what they wanted. The only case in which it would make some sense to use that line with empty curlies would be if... 4. the entire thing is just that bad.	[reply] [d/l]
Re^4: Perl - Source code review by Jenda (Abbot) on Nov 13, 2014 at 11:23 UTC
Re^2: Perl - Source code review by Anonymous Monk on Nov 13, 2014 at 08:24 UTC
The project is not small. Do you really think someone will review it for free? Hmm, how big is it? My anonymous proxy turned the zipfile into a bigger .exe ... I did not download :)	[reply]
Re: Perl - Source code review by Anonymous Monk on Nov 13, 2014 at 10:02 UTC
After adding 'use strict; use warnings' to the 'project2' file, this is what I got: Global symbol "@ReadConfigValues" requires explicit package name at pr +oject2 line 23. Global symbol "@ReadConfigValues" requires explicit package name at pr +oject2 line 29. Global symbol "$i" requires explicit package name at project2 line 47. Global symbol "$i" requires explicit package name at project2 line 47. Global symbol "@ReadConfigValues" requires explicit package name at pr +oject2 line 47. Global symbol "$i" requires explicit package name at project2 line 47. Global symbol "@ServiceList" requires explicit package name at project +2 line 48. Global symbol "@ReadConfigValues" requires explicit package name at pr +oject2 line 48. Global symbol "$i" requires explicit package name at project2 line 48. Global symbol "@LogFileList" requires explicit package name at project +2 line 49. Global symbol "@ReadConfigValues" requires explicit package name at pr +oject2 line 49. Global symbol "$i" requires explicit package name at project2 line 49. Global symbol "@TempLogFileList" requires explicit package name at pro +ject2 line 55. Global symbol "@TempServiceList" requires explicit package name at pro +ject2 line 56. Global symbol "@TempLogFileList" requires explicit package name at pro +ject2 line 58. Global symbol "@LogFileList" requires explicit package name at project +2 line 59. Global symbol "@TempLogFileList" requires explicit package name at pro +ject2 line 59. Global symbol "$i" requires explicit package name at project2 line 60. Global symbol "$i" requires explicit package name at project2 line 60. Global symbol "@LogFileList" requires explicit package name at project +2 line 60. Global symbol "$i" requires explicit package name at project2 line 60. Global symbol "@LogFileList" requires explicit package name at project +2 line 61. Global symbol "$i" requires explicit package name at project2 line 61. Global symbol "@LogFileList" requires explicit package name at project +2 line 61. Global symbol "$i" requires explicit package name at project2 line 61. Global symbol "@ServiceList" requires explicit package name at project +2 line 63. Global symbol "@TempServiceList" requires explicit package name at pro +ject2 line 66. Global symbol "@ServiceList" requires explicit package name at project +2 line 67. Global symbol "@TempServiceList" requires explicit package name at pro +ject2 line 67. Global symbol "$i" requires explicit package name at project2 line 68. Global symbol "$i" requires explicit package name at project2 line 68. Global symbol "@ServiceList" requires explicit package name at project +2 line 68. Global symbol "$i" requires explicit package name at project2 line 68. Global symbol "@ServiceList" requires explicit package name at project +2 line 69. Global symbol "$i" requires explicit package name at project2 line 69. Global symbol "@ServiceList" requires explicit package name at project +2 line 69. Global symbol "$i" requires explicit package name at project2 line 69. Global symbol "@ServiceList" requires explicit package name at project +2 line 75. Global symbol "@LogFileList" requires explicit package name at project +2 line 75. Global symbol "@ServiceList" requires explicit package name at project +2 line 76. Global symbol "$ThisFile" requires explicit package name at project2 l +ine 87. Global symbol "$ThisFile" requires explicit package name at project2 l +ine 88. Global symbol "$ThisService" requires explicit package name at project +2 line 89. Global symbol "$ThisFile" requires explicit package name at project2 l +ine 89. Global symbol "$ThisService" requires explicit package name at project +2 line 90. Global symbol "$ThisService" requires explicit package name at project +2 line 90. Global symbol "@AllServices" requires explicit package name at project +2 line 91. Global symbol "$ThisService" requires explicit package name at project +2 line 91. Global symbol "@ServiceList" requires explicit package name at project +2 line 98. Global symbol "@ServiceList" requires explicit package name at project +2 line 98. Global symbol "@ServiceList" requires explicit package name at project +2 line 100. Global symbol "@AllServices" requires explicit package name at project +2 line 100. Global symbol "@LogFileList" requires explicit package name at project +2 line 101. Global symbol "@LogFileList" requires explicit package name at project +2 line 102. Global symbol "@ServiceList" requires explicit package name at project +2 line 102. Global symbol "$LogFile" requires explicit package name at project2 li +ne 131. Global symbol "@LogFileList" requires explicit package name at project +2 line 131. Global symbol "$FileText" requires explicit package name at project2 l +ine 134. Global symbol "$FileText" requires explicit package name at project2 l +ine 135. Global symbol "$LogFile" requires explicit package name at project2 li +ne 135. Global symbol "$FilterText" requires explicit package name at project2 + line 137. Global symbol "$FilterText" requires explicit package name at project2 + line 139. Global symbol "$LogFile" requires explicit package name at project2 li +ne 139. Global symbol "$Command" requires explicit package name at project2 li +ne 141. Global symbol "$FileText" requires explicit package name at project2 l +ine 141. Global symbol "$FilterText" requires explicit package name at project2 + line 141. Global symbol "$LogFile" requires explicit package name at project2 li +ne 141. Global symbol "$Command" requires explicit package name at project2 li +ne 142. Global symbol "$Command" requires explicit package name at project2 li +ne 143. Global symbol "$LogFile" requires explicit package name at project2 li +ne 150. Global symbol "@LogFileList" requires explicit package name at project +2 line 150. Global symbol "$FileText" requires explicit package name at project2 l +ine 152. Global symbol "$FileText" requires explicit package name at project2 l +ine 153. Global symbol "$LogFile" requires explicit package name at project2 li +ne 153. Global symbol "$FilterText" requires explicit package name at project2 + line 155. Global symbol "$FilterText" requires explicit package name at project2 + line 156. Global symbol "$LogFile" requires explicit package name at project2 li +ne 156. Global symbol "$Command" requires explicit package name at project2 li +ne 159. Global symbol "$FileText" requires explicit package name at project2 l +ine 159. Global symbol "$FilterText" requires explicit package name at project2 + line 159. Global symbol "$LogFile" requires explicit package name at project2 li +ne 159. Global symbol "$Command" requires explicit package name at project2 li +ne 160. Global symbol "$Command" requires explicit package name at project2 li +ne 161. Execution of project2 aborted due to compilation errors. [download] My review: the source code is really bad. Use strict, warnings, use File::Spec, File::Temp and other standard library modules. Don't use two-argument form of open, don't use bareword filehandles, etc.	[reply] [d/l]
Re: Perl - Source code review by CountZero (Bishop) on Nov 13, 2014 at 18:06 UTC
Actually, the assignment is not about Perl at all. Perl is just a convenient language to carry the assignment. The project is all about finding some vulnerabilities, in this case a race condition, and suggesting some solutions to avoid this vulnerability. CountZero A program should be light and agile, its subroutines connected like a string of pearls. The spirit and intent of the program should be retained throughout. There should be neither too little or too much, neither needless loops nor useless variables, neither lack of structure nor overwhelming rigidity." - The Tao of Programming, 4.1 - Geoffrey James My blog: Imperial Deltronics	[reply]
Re^2: Perl - Source code review by Anonymous Monk on Nov 13, 2014 at 18:28 UTC
Actually, the assignment is not about Perl at all. Perl is just a convenient language to carry the assignment. Maybe, but Perlmonks is not about doing other people's homework... but about Perl :) The project is all about finding some vulnerabilities, in this case a race condition, and suggesting some solutions to avoid this vulnerability. Anyway... if the OP is still here: one of the race conditions is very likely to be in the part where tmp files are created... Solution: use File::Temp.	[reply]
Re: Perl - Source code review by oiskuu (Hermit) on Nov 13, 2014 at 18:42 UTC
Project TempDir creation is insecure. The attacker will (a) observe or guess the cron process PID, (b) prepare a directory wherein a symlink service_foo -> some strategic path exists, (c) attempt to rename this to the cron job's TempDir at the critical moment before the invocation of mkdir but after the `-d -e` checks. Succeeding that, the privileged process will go on to overwrite some system file of your choosing. For the shoddy code in question, the attack is even simpler: neither rmdir nor unlink can remove a directory with files, so all the attacker needs to do is prepare 32766 tempdirs and wait for the hour to strike. As for repair suggestions, mkdtemp, checking return values, and so on. You're welcome. Edit. The vulnerable section: `$TempDir = $TempDir. "project2." . $$ . "/"; if ( -d $TempDir ) { rmdir ($TempDir); #print "\nRemoving Existing Temp Dir"; } if ( -e $TempDir ) { unlink ($TempDir); #print "\nRemoving the tempdir if it is existing as a file\n"; } mkdir ($TempDir,0700); print "\nMaking Temp Dir: " . $TempDir . "\n";` [download]	[reply] [d/l] [select]
Re^2: Perl - Source code review by Anonymous Monk on Nov 13, 2014 at 19:31 UTC
The attacker will (a) observe or guess the cron process PID,... The attacker has access to the machine? Yeah game over already, no matter what the program/module does	[reply]