search

coreybutler / node-windows

Windows support for Node.JS scripts (daemons, eventlog, UAC, etc).
Other
2.78k stars 357 forks source link

Prevent security scanner from killing wrapper with ECONNRESET #277

Open ChrisMiami opened 3 years ago

ChrisMiami commented 3 years ago

wrapper.js is continually killed by a security penetration test that causes ECONNRESET errors on the socket. Handling the socket error prevents a server error which forestalls cycling the app. Since we don't care about the connections anyway, I log the error but ignore it.

In server.on('error'), I added a server.close() because server errors do not automatically close the server, so it was probably leaking servers.

coreybutler commented 3 years ago

@ChrisMiami - I don't personally come across this scenario, but the fix seems fine/logical. My one concern is cluttering the logs with ignored log messages. In particular, if someone is retroactively scanning logs, will they be scanning through alot of noise? If it's more of a signal of a resolvable problem in the underlying app, then I'm fine with it as-is. If not, it seems prudent to have a configuration flag to toggle the messaging.

ChrisMiami commented 3 years ago

@coreybutler - this makes eminent sense, but although the event has been trapped to make it a non-issue, I'd guess it's something that more people would want to see than not. In my case it was a proactive internal security scanner, but in other cases it could signal a breach or DoS attack because what else would be connecting to that socket?

Still, I'll add a separate log-level filter option (different PR?) to only log events at or above the listed level, with doc update. Is that a good solution?

coreybutler commented 2 years ago

@ChrisMiami I'm not sure if this is still something you're interested in, given the age. I've been going through old PR's in a number of projects and came across this. I'd still be open to merging this one, and yes, having a separate log-level filter option would be a good solution.

lsh246 commented 2 years ago

@coreybutler We're running into this issue as well and would love to have this PR merged. Could we create the logging level feature as a separate issue?

edumansky commented 1 year ago

@coreybutler We're also hitting this same issue in production FYI

lsh246 commented 1 year ago

@coreybutler still interested in this, FWIW.

jkomoda commented 2 months ago

@ChrisMiami may you please allow me to push changes to your branch? I've made the logging change and we want to merge this ASAP. Otherwise, I'll create another PR