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

Remove local file additions in setup-win.c #113

Merged
merged 1 commit into from
Mar 6, 2014
Merged

Remove local file additions in setup-win.c #113

merged 1 commit into from
Mar 6, 2014

Conversation

awiddersheim
Copy link
Member

In my opinion adding these should be a user decision and shouldn't get done by default.

In my opinion adding these should be a user decision and shouldn't get done by default.
@ddpbsd
Copy link
Member

ddpbsd commented Mar 6, 2014

I disagree, if those files exist they should be monitored by default. Most users won't know if those logs even exist.

@labrown
Copy link
Contributor

labrown commented Mar 6, 2014

Why just those two files? Seems like there might be a whole set of files created by various antivirus/antimalware programs that "should" be monitored.

Feels like a "spooky action at a distance" sort of thing.

@awiddersheim
Copy link
Member Author

I just feel like using that logic leads to a slippery slope to go down. At what point do you draw the line where you stop making decisions for the user and letting the user decide?

If you want to keep them it would be better to add them to the recommended default configuration file (https://github.com/ossec/ossec-hids/blob/master/src/win32/ossec.conf )instead of hardcoding them as getting added to every install.

ddpbsd added a commit that referenced this pull request Mar 6, 2014
Remove local file additions in setup-win.c
@ddpbsd ddpbsd merged commit e5fcda7 into ossec:master Mar 6, 2014
@ddpbsd
Copy link
Member

ddpbsd commented Mar 6, 2014

Ok, looking at it again I can agree with removing this. I don't see the pfirewall log file on my system, and who knows how old symantec 7.5 is.

@mstarks01
Copy link
Contributor

awiddersheim, I am pleased to see you contributing so much, but I disagree with this change. I think it's reasonable that a user would expect ossec to start monitoring logs it has decoders and rules for, without user intervention. That's the value that the software brings.

In the case of the firewall log, the default locations can be found here: http://technet.microsoft.com/en-us/library/cc947815(v=ws.10).aspx. We need to add a potential location, not remove support for it.

I think the proper way to handle this is for the agent or the installer to search the default locations for logs it understands and add them to the configuration, all without user intervention.

@awiddersheim
Copy link
Member Author

What is wrong with adding both locations to the default ossec.conf for win32 platforms? At least there the user has the chance to configure whether they want it or not before compiling/creating the OSSEC installer executable.

I'm not saying OSSEC shouldn't monitor these log files. I'm saying where it is configured to do that is in the wrong place.

@mstarks01
Copy link
Contributor

They should be in ossec.conf, but only if they exist on the system. That's the job of setup-win.c in this case--to see if they are there and add them to be monitored. A typical Windows user isn't going to compile the binary themselves. They will just run the installer and expect things to work.

@awiddersheim
Copy link
Member Author

In a perfect world the OSSEC installer would be this super awesome logic system that looked at all of the things your system might be doing to create a logical ossec.conf. I agree, that this would be cool but we are far from that perfect world at this point.

I already have ~10 PRs just to fix some other large issues and have a list of ~20 or so more things. Just to clean up the amount of terrible things with the win32 stuff now is a pretty large amount of work. Probably should clean all of that up first before we start to build out what you are asking for.

What you are asking for is more of a future roadmap item. There isn't anything in the OSSEC code that does this now that I know of. Even on NIX based systems. Maybe in a few months if things get straightened out I can start to entertain this idea and work on it.

Probably be easier to create a powershell/python script that users can run to inspect their systems and create a base ossec.conf file. Be much easier than doing it in C. Than you could add that as a run after for the ossec-installer.exe similar to how the IIS setup stuff gets done.

In the meantime it is of my opinion that if you feel it is imperative to have these files monitored on every system than you should add those locations to the default win32 config. I'm not saying every user should do that and have to re-build the installer executable. I'm saying feel free to make that change in the Github repo so all future releases will have them included.

Correct me if I'm wrong but having OSSEC looking at log files that don't exist isn't harmful in anyway. It will just spit out warnings to the log file.

@mstarks01
Copy link
Contributor

I agree that the method for adding files could be better. But why deprecate something that would add a potentially useful file to ossec.conf when we have no better method in place currently? And to add it to ossec.conf for everyone and intentionally cause a soft failure if the file doesn't exist, when this isn't an issue with the way it currently gets handled, doesn't make sense to me. I think we should leave the current method in place until we have something better.

Anyway, that's my .02. I respect that we have different opinions on the best way forward.

@awiddersheim
Copy link
Member Author

On the systems we run here I don't care to monitor the Windows FW log. The problem is with this system it adds it automatically and I have no choice in the matter. All I can do is wait until it is installed and manually remove it later.

Atleast now I can add my own default ossec.conf rebuild the ossec-installer.exe and be fine. I don't like the "we made some decisions for you whether you like it or not" idea. The worst part is this decision isn't even obvious. It's hidden in some C code.

@ddpbsd
Copy link
Member

ddpbsd commented Mar 6, 2014

It's no longer in the C code. Now whether the firewall entry gets added to the default ossec.conf is a different issue.
I'm leaning towards adding it to the default ossec.conf, but commenting it out. This draws attention to it for users who may not be aware of it, but also gives users the choice as to whether it is enabled or not.

@mstarks01
Copy link
Contributor

You may or may not know this, but you can extract the installer with 7zip and place your own ossec.conf in there. It's just an archive, really.

@awiddersheim
Copy link
Member Author

I didn't know that. Thanks.

@jbcheng
Copy link
Contributor

jbcheng commented Mar 6, 2014

The add-localfile.exe statements were first added in October 2006 by dcid,
based on contribution by Joe Dance (
https://bitbucket.org/dcid/ossec-hids/commits/1c8a91aec7b3c0ed22733c3e7decfca62f7270fe#chg-src/win32/setup-win.c
)

It was hard coded in C to check whether the local file exists and if yes,
to modify the Windows Agent ossec.conf file syscheck section. It got the
job done at that time.

In general, I do not like removing existing features unless a new one is in
place.
However, in this case, the hard coded file paths are very old and whoever
still relying on Symantec AV 7.5 can keep using the pre-2.7.1 release.
They don't need to upgrade to the next OSSEC release Windows agent. So,
I agree with removing these two lines.

On Thu, Mar 6, 2014 at 12:23 PM, mstarks01 [email protected] wrote:

I agree that the method for adding files could be better. But why
deprecate something that would add a potentially useful file to ossec.conf
when we have no better method in place currently? And to add it to
ossec.conf for everyone and intentionally cause a soft failure if the file
doesn't exist, when this isn't an issue with the way it currently gets
handled, doesn't make sense to me. I think we should leave the current
method in place until we have something better.

Anyway, that's my .02. I respect that we have different opinions on the
best way forward.

Reply to this email directly or view it on GitHubhttps://github.com//pull/113#issuecomment-36932366
.

JB Cheng le

@mstarks01
Copy link
Contributor

I am fine with putting the pfirewall.log lines in the ossec.conf but commented out for now. I think that's a reasonable compromise until we have something better in place.

@jrossi
Copy link
Member

jrossi commented Mar 6, 2014

Being a slave to backward compatibily creates more problems then it is worth. But I think about it like this I think the Installer between minor version should be reasonably the same, and this is something we started to codify in oRFC : 1. And the Evolution of Public Contracts section. Maybe this should be revisited for install conf. Anywho

Removing manic is always good and having the reinstall always add back in makes for unhappy people.

Let's just rev to next version so people know more then Minor changes have happened.

@jrossi jrossi added this to the ossec-hids-2.8 milestone Mar 8, 2014
@jrossi jrossi added enhancement and removed invalid labels Mar 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants