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.

Sign up for GitHub

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

Jump to bottom

[Websocket Passive Scan] Script Mechanism #1718

Merged
merged 5 commits into from
Jul 4, 2019
Merged

[Websocket Passive Scan] Script Mechanism #1718

merged 5 commits into from
Jul 4, 2019

Conversation

eakirtas
Copy link
Contributor

@eakirtas eakirtas commented Jul 13, 2018 •
edited
Loading

  • [WS Passive Scan] Add Passive scan mechanism (for Scripts only)

The mechanism runs starts a background thread and looping until telling that to stop. Background thread has a Thread Safe Queue in order to add (produce) and scan (consume) messages. To take the messages background thread make queries in the database (message by message). The whole process managed by WebSocketManager in which WebSocketPassiveScanners and plugins are stored.

Also, Add a new Script Plugin for WebSocket Scanning with the appropriate
interface.

TODO:

  • Decorate the scanner with a class which implements hashcode and equals
  • Remove the WebSocketScanner.applyScanToMessage
  • Rename getPluginId to getId
  • Replace ConcurrentLinkedHashSet with another class from library java.util.concurrent (like CopyOnWriteArraySet)
  • Fix errors triggered by using WebSocket Passive Scan with ZAP HUD (more-info)
  • Add helper for Passive scan plugins
  • Make an Icon
  • [WS-Passive Scan] WebSocket Alert Wrapper

A "fake" alert mechanism for WebSockets. WebSocketAlertWrapper uses the core Alert in order to represent the WebSocketAlerts. The WebSocketWrapper creates an Alert by replacing other info with WebSocketMessage readable payload and http message with http handshake.

TODO:

  • Add alert builder
  • [WS-Passive Scan] Template Script with Javascript.
  • [WS Passive Scan] Template Script with Python

@eakirtas eakirtas force-pushed the websocket_pscan_alerts branch 5 times, most recently from b92ab30 to 8c921b9 Compare July 16, 2018 13:34
@eakirtas
Copy link
Contributor Author

Some comments which discussed with @thc202 in IRC addressed

psiinon
psiinon reviewed Aug 9, 2018
View reviewed changes
Copy link
Member

@psiinon psiinon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to try out the code - aiming to do that asap ;)

psiinon
psiinon requested changes Aug 9, 2018
View reviewed changes
Copy link
Member

@psiinon psiinon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried it and it works really well - good work!
A couple of minor points from me, as mentioned above.

@eakirtas eakirtas force-pushed the websocket_pscan_alerts branch 2 times, most recently from 231d746 to a239cb5 Compare September 28, 2018 13:22
@eakirtas
Copy link
Contributor Author

@psiinon 's and LGTM suggestion addressed.

@eakirtas eakirtas force-pushed the websocket_pscan_alerts branch from a239cb5 to 65942d2 Compare March 12, 2019 18:42
@psiinon
Copy link
Member

psiinon commented Mar 12, 2019

This pull request introduces 9 alerts when merging 65942d2 into 7cd4f60 - view on LGTM.com

new alerts:

  • 9 for Missing variable declaration

Comment posted by LGTM.com

kingthorin
kingthorin reviewed Mar 12, 2019
View reviewed changes
Copy link
Member

@kingthorin kingthorin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of feedback after a quick skim....

@eakirtas eakirtas force-pushed the websocket_pscan_alerts branch from 65942d2 to e7c2af0 Compare March 15, 2019 10:18
@eakirtas eakirtas force-pushed the websocket_pscan_alerts branch 2 times, most recently from 68de77c to 8553678 Compare March 15, 2019 12:14
@eakirtas
Copy link
Contributor Author

The above minor issues addressed. Further issues is added to the initial comment under TODO section.

@eakirtas eakirtas mentioned this pull request Mar 15, 2019
Merged
@kingthorin
Copy link
Member

kingthorin commented Mar 15, 2019 •
edited
Loading

Further issues is added to the initial comment under TODO section.

I've updated the task list, to me it seems 2/4.

Edit: Disregard, I was thinking you meant you addressed more items, but now that I looked at the history you added a 4th item 👍 (oops)

@eakirtas eakirtas force-pushed the websocket_pscan_alerts branch from 1b0d6c1 to 94a1484 Compare June 17, 2019 14:26
@eakirtas
Copy link
Contributor Author

Comments have been resolved. I add a getEnabledIterator in WebSockePasssiveScanManager in order to be used by WebSocketPassiveThread

PS: I used to squash commits before pushed them, however I realize that it is pain for review. So i am going to squash them before get merged.

@eakirtas eakirtas force-pushed the websocket_pscan_alerts branch from 98e98f9 to 7603293 Compare June 21, 2019 08:16
thc202
thc202 reviewed Jun 27, 2019
View reviewed changes
@eakirtas eakirtas force-pushed the websocket_pscan_alerts branch from 9718a29 to 5b067ce Compare June 28, 2019 13:41
@lgtm-com
Copy link

lgtm-com bot commented Jun 28, 2019

This pull request introduces 1 alert when merging 1a7c2ad into f4b11d4 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

@eakirtas eakirtas force-pushed the websocket_pscan_alerts branch from 9ce0a1d to 073e88c Compare July 1, 2019 19:18
@lgtm-com
Copy link

lgtm-com bot commented Jul 1, 2019

This pull request introduces 1 alert when merging 073e88c into f4b11d4 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

@thc202
Copy link
Member

thc202 commented Jul 1, 2019

The above alert is a FP.

@thc202
Copy link
Member

thc202 commented Jul 3, 2019

@ManosMagnus thanks! Let us know when it's tidied up for merge.

@eakirtas eakirtas force-pushed the websocket_pscan_alerts branch 2 times, most recently from 8b146f5 to 07a4577 Compare July 4, 2019 09:19
@eakirtas eakirtas force-pushed the websocket_pscan_alerts branch from 07a4577 to 60e1184 Compare July 4, 2019 09:20
@eakirtas
Copy link
Contributor Author

eakirtas commented Jul 4, 2019

Commits were tidied up for merge. Thank for help!

@eakirtas eakirtas changed the title [WIP] [Websocket Passive Scan] Script Mechanism [Websocket Passive Scan] Script Mechanism Jul 4, 2019
@eakirtas eakirtas force-pushed the websocket_pscan_alerts branch from 7e2790c to 1d4e2fd Compare July 4, 2019 09:57
@thc202 thc202 merged commit 193d672 into zaproxy:master Jul 4, 2019
@kingthorin
Copy link
Member

Whoop whoop, thanks @ManosMagnus!

@zaproxy zaproxy deleted a comment from lgtm-com bot Jul 4, 2019
@eakirtas eakirtas deleted the websocket_pscan_alerts branch July 11, 2019 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kingthorin kingthorin kingthorin left review comments

@psiinon psiinon psiinon approved these changes

@thc202 thc202 thc202 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eakirtas @psiinon @kingthorin @thc202