Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Open web analytics 1.7.3 remote code execution #17754
Open web analytics 1.7.3 remote code execution #17754
Changes from 9 commits
b37be28
76b05a7
7068d4c
f0dbf54
d59175a
7b0a54b
3847c41
ae7ca16
14b5c08
ee95eb2
94ceeb0
2de5371
614f4b6
8518563
69839d1
e66fd8f
38511f4
3f7f28d
94e9504
ddd594a
3196a52
cc4e455
3113149
ac6e947
9e64f02
cfaad7f
c0ee250
2ce3aee
dff139d
8db10af
887551b
e160e51
86f4a16
2310b0d
897aaf9
d72d47e
bb9e214
103def7
0cbebc8
ee0334d
cea8aa8
3bf60a5
d06e2d9
ac72c12
027793c
3baa894
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be a good idea to also reference the original write up: https://devel0pment.de/?p=2494
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of
send_request_cgi
should always be checked since it can benil
. Callingres.code
will break here ifres
isnil
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The failure reason should be
UnexpectedReply
in this case. Also, there is a little typooccured
->occurred
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment, the code should check if
cache_request
isnil
before callingcache_request.code
.Note this comment is also valid for any value returned by
send_request_cgi
in the code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot find where
print_message
is defined. Do you mean usingprint_status
,vprint_status
,print_good
orvprint_good
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to backup the original value?
If so, you can add the logic to restore config values to the
cleanup
method, which will be automatically called when the exploit terminates. Here is an example.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to find a way to retrieve the original value, but after giving it a first shot earlier I don't think that will be possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should the HTTP response code be checked to make sure it was successful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question about the HTTP response code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the
=
are included in this string, they're not getting matched by the\w
. You could change that so they are included, removing the need to calculate and add them yourself on L273-L274 by changing the regex to:The
\w
should also be updated to include the additional characters that base64 can include in the encoding such as+
and/
depending on the character set/flavor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! I updated the regex to your suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can conclude it is not vulnerable here, the failure reason should be
NotVulnerable
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here,
match
can returnnil
and calling[1]
could break. Also, it looks like the local variableupdate_nonce
is not necessary and can be removed.