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

Add AUDIO_EFFECTS to distinguish from others #10744

Merged
merged 3 commits into from Oct 5, 2018

Conversation

wvu
Copy link
Contributor

@wvu wvu commented Oct 4, 2018

Constant Description
SCREEN_EFFECTS Module may show something on the screen (Example: a window pops up)
AUDIO_EFFECTS Module may cause a noise (Examples: audio output from the speakers or hardware beeps)
PHYSICAL_EFFECTS Module may produce physical effects (Examples: the device makes movement or flashes LEDs)

Updates #10707.

@jrobles-r7
Copy link
Contributor

wasn't there physical effects?

@bcoles
Copy link
Contributor

bcoles commented Oct 4, 2018

Looks good to me.

I think a separate constant for AUDIO is beneficial, but do we need/want this distinction between software/hardware audio?

This line gets blurry, as some systems will emit a beep from the internal speaker if no audio device is configured.

# Module may cause software to output audio from the speakers (Example: the app plays music)
AUDIO_EFFECTS          = 'audio-effects'
# Module may produce physical effects in hardware (Examples: LED or LCD changes or hardware beeps)
PHYSICAL_EFFECTS       = 'physical-effects'

@wvu
Copy link
Contributor Author

wvu commented Oct 4, 2018

I made the distinction because I assumed we wanted to be clear and break down the constants, since we've been going that route. Hardware vs. software was one such distinction. I have a proposed compromise that I think works better.

@h00die
Copy link
Contributor

h00die commented Oct 4, 2018

Out of curiosity is there any modules which currently have an audible effect?

@wvu
Copy link
Contributor Author

wvu commented Oct 4, 2018

Probably. https://holeybeep.ninja/ comes to mind as a vuln. That's a hardware beep, but it's all hardware in the end, usually with some software component. The line is indeed blurry.

@wvu
Copy link
Contributor Author

wvu commented Oct 4, 2018

I updated the constant descriptions (though they're only comments right now), and I think I am happy with them now.

@bcoles
Copy link
Contributor

bcoles commented Oct 4, 2018

@h00die The lastore_daemon_dbus_priv_esc.rb module installs a system package, resulting in an audible sound.

As per the documentation:

It may cause audio and/or graphical signals confirming the installation of the payload package.

@@ -82,7 +82,9 @@ module Msf
ACCOUNT_LOCKOUTS = 'account-lockouts'
# Module may show something on the screen (Example: a window pops up)
SCREEN_EFFECTS = 'screen-effects'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wchen-r7, @bcoles: Where do we want to draw the line with screen effects vs. visual effects vs. what is physical?

Copy link
Contributor

Choose a reason for hiding this comment

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

Screen being pop up boxes, relics displayed in a browser
Physical being cdrom ejection, scada valve movement
Visual being changing an external LED number display

That's how I'd think, but visual vs screen took a little bit of thinking about

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So are we adding another constant?

Copy link
Contributor

@wchen-r7 wchen-r7 Oct 5, 2018

Choose a reason for hiding this comment

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

So for me that means:

SCREEN_EFFECTS: Something the user can see on the screen that allows them to realize the machine is being exploited.
PHYSICAL_EFFECTS: It involves a moving object.
AUDIO_EFFECTS: A noise or sound that allows the user to realize the machine is being exploited.
VISUAL_EFFECT: Kind of sounds like a sub-category of screen_effects. A little hard to draw the line for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also add that physical could be a temperature as well. Like a Crock-Pot :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I like sinn3r's summary of constants. I think these are solid (note: small edits):

# Module may show something on the screen (Example: a window pops up)
SCREEN_EFFECTS   = 'screen-effects'
# Module may cause a noise (Example: output audio from the speakers or hardware beep)
AUDIO_EFFECTS    = 'audio-effects'
# Module may produce physical effects (Example: the device moves)
PHYSICAL_EFFECTS = 'physical-effects'

The question remains whether an external LED or LCD falls within SCREEN_EFFECTS or a new VISUAL_EFFECTS category.

Copy link
Contributor Author

@wvu wvu Oct 5, 2018

Choose a reason for hiding this comment

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

Yeah, hardware beeps should go in AUDIO_EFFECTS. We're trying to be specific and at the same time not confusing, heh.

I think I'll change "the device moves" to "the device makes movement," since the implication is that there is movement, not necessarily locomotion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@h00die: I removed that because it potentially puts us in hot water. No pun intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've decided to leave VISUAL_EFFECTS out for now. It's a bit too nuanced to be useful right now, IMHO. I think we are in alignment with the current designations.

@wchen-r7
Copy link
Contributor

wchen-r7 commented Oct 5, 2018

Nice. Ok this looks good. It looks like this PR has everyone's blessing, so I will go ahead and land this. I'll also remember to document what everyone says here so we're all happy with the definitions. Thank you everyone!

@wchen-r7 wchen-r7 merged commit 6efadb5 into rapid7:master Oct 5, 2018
@wchen-r7
Copy link
Contributor

wchen-r7 commented Oct 5, 2018

Release Notes

This adds the AUDIO_EFFECTS label to keep track of side effects, such as audio output from speakers or hardware beeps.

@wvu
Copy link
Contributor Author

wvu commented Oct 5, 2018

Thank you, lovely pedants, myself included. :-)

@wvu wvu deleted the feature/traits branch October 5, 2018 21:56
jmartin-tech pushed a commit to jmartin-tech/metasploit-framework that referenced this pull request Oct 24, 2018
@gdavidson-r7 gdavidson-r7 added the rn-enhancement release notes enhancement label Nov 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants