Fix manage agents keys #336

awiddersheim · 2014-10-06T00:26:11Z

Create temporary files in a secure manner on both NIX and Windows
platforms that will be used to write client.keys and ossec.conf data to.
Then use atomic operations where possible to replace the current
versions when making updates.

On Windows, decided to hard code the location of COMSPEC instead of
pulling it from an environment variable each time. It is highly unlikely
the location of this will change but may re-examine in the future.

Since temporary files are now a possibility on agent only installs I
updated the Windows installer to include a tmp directory. Need to figure
out what is involved with doing the same on the NIX side.

awiddersheim · 2014-10-06T00:36:42Z

Worked a lot on this over the weekend to hopefully implement everything requested in #292. This now creates temporary files with proper permissions, writes the data to those files and replace the existing files in an atomic operation (hopefully).

Tested on RHEL6, Windows Server 2003, Windows Server 2008R2, Windows Server 2012 and Windows Server 2012R2.

I made a lot of guesses on how the Windows stuff will/should work. There are still some times where atomic operations don't happen. Specifically in the UI when the ossec.conf gets updated. A backup copy is made before the new version is put into place. This is done by doing a rename() opeartion. A CopyFile() would be better but I found that it doesn't handle copying permissions as well. I kind of ran out of time/stopped caring and I didn't want to get rid of this functionality yet so I just left that the way it was.

There was also some progress toward #182. Atleast, I hope so. I'm hoping @mstarks01 can review this a bit further but the new client.keys and ossec.conf files now have SYSTEM added to the permissions like so:

Again, I am hoping that is a step in the right direction to achieving #182. Still a lot of work to do on that front though.

Also, I am hoping this fixes #303. I think it should since cacls and Administrator are no longer used. This should now be language independent but more testing is needed to confirm.

Finally, as mentioned in the pull request, I opted toward having a tmp directory for agent only installations. I implemented the necessary stuff to make that happen on the Windows side but I'm guessing things will need to be done on the NIX side as well with install.sh. I'm not sure exactly where that would need to get done so if someone could point me in the right direction it would be appreciated.

If this pull request does get accepted and we opt to keep the tmp directory than it is likely package maintainers like @atomicturtle will need to make updates to their stuff for the next release as well.

jrossi · 2014-10-06T02:50:40Z

Wow - this good stuff :) some research on my end; great stuff. Thank you much.

As for tmp for I agree makes sense. To add this just look at InstallServer.sh and InstallAgent.sh.

awiddersheim · 2014-10-06T03:13:54Z

Thanks. Yeah, InstallAgent.sh looks like what I want. I will make updates to that tomorrow morning. Looks like travis-ci is down for maintenance so this hasn't gone through a test build yet.

awiddersheim · 2014-10-06T13:40:52Z

This should be all that is necessary hopefully. Let me know what you think.

awiddersheim · 2014-10-06T19:10:38Z

Sorry. Few more small fixes. Hopefully that is it but would really like @mstarks01 to review the permissions I have setup to see if I'm headed in the right direction or not so I can made the necessary adjustments.

awiddersheim · 2014-10-07T19:03:22Z

I added the tmp stuff to the InstallAgent.sh script but I'm guessing the recent Makefile stuff makes that change moot? Do I need to update this pull request?

jrossi · 2014-10-07T19:36:03Z

Yeah I would the new system is much better :) sorry about the double work.

awiddersheim · 2014-10-07T19:53:14Z

Actually, it looks like everything is already there.

https://github.com/ossec/ossec-hids/blob/master/src/Makefile#L280
https://github.com/ossec/ossec-hids/blob/master/src/Makefile#L395

jrossi · 2014-10-07T19:54:38Z

@cgzones is winning :)

jrossi · 2014-10-09T12:30:40Z

Can you merge in master to allow for clean merging and travis ci? Thank you

awiddersheim · 2014-10-09T13:42:12Z

Sorry the merge went a little poorly. Maybe I should have rebased instead. Only seems like the install files were affected. I can redo this pull request if you want.

jrossi · 2014-10-09T13:51:12Z

It's fine the issue looks to be with .old files we will be deteleting soon :)

Create temporary files in a secure manner on both NIX and Windows platforms that will be used to write client.keys and ossec.conf data to. Then use atomic operations where possible to replace the current versions when making updates. On Windows, decided to hardcode the location of COMSPEC instead of pulling it from an environment variable each time. It is highly unlikely the location of this will change but may re-examine in the future. Since temporary files are now a possibility on agent only installs I updated the Windows installer to inclue a tmp directory. Need to figure out what is involved with doing the same on the NIX side.

This code should make the temporary file names a bit more consistant with what gets set in the global constants. This way if those ever change the temporary files should as well and hopefully continue to make sense as to what they do.

This lets the user see progress and where errors/warnings are happening a bit easier. It also is similar to how the output done by the Make stuff on the NIX side.

awiddersheim · 2014-10-09T14:12:31Z

I did a rebase. Seems okay. I never really run into these more "complicated" situtaions with git that often so I saw an opportunity to try and learn something.

cgzones · 2014-10-10T12:19:37Z

I like the rebase function too.

About the char *basename_ex(char *path) function for unix:
You implemented it with return(basename(path));, but then non-gnu version might modify its argument (http://linux.die.net/man/3/basename) and if you call it with a constant string (which a expanded macro is) it might crash or result in ub).
Maybe use something like:

const char *my_basename(const char *path) {
    const char *last = NULL, *next_to_last = NULL, *it = path;

    while ((it = strchr(it, '/')) != NULL) {
        next_to_last = last;
        last = it;
        it++;
    }

    return last ? ( *(last+1) != '\0' ? last+1 : ( next_to_last ? next_to_last+1 : last ) ) : path;
}

It matches not quiet the gnu specification, but almost:
official (http://linux.die.net/man/3/basename):

path        basename
"/usr/lib"    "lib"
"/usr/"       "usr"
"usr"         "usr"
"/"           "/"
"."           "."
".."          ".."

my_basename:

path         basename
"/usr/lib"    "lib"
"/usr/"       "usr/"
"usr"         "usr"
"/"           "/"
"."           "."
".."          ".."

or

maybe replace

char *keys_file = basename_ex(AUTH_FILE);

with

char auth_file_tmp[] = AUTH_FILE;
char *keys_file = basename_ex(auth_file_tmp);

@cgzones

Avoid possible segfault by creating a non-constant copy of macros to pass to basename_ex() since there is the possibility the arguments passed could be modified. This is one of the suggestions made in the documentation. Thanks to @cgzones for catching this issue.

awiddersheim · 2014-10-11T18:50:46Z

@cgzones Good catch. I decided to go with your second suggestion since it is mentioned in the documentation you linked to.

awiddersheim · 2014-10-19T19:06:28Z

Anyway this can get merged into master soon? This is getting increasingly harder to merge as time goes on.

jrossi · 2014-10-19T19:07:20Z

Sorry yeah reviewed it the other day did not merge. My bad.

On Oct 19, 2014, at 3:06 PM, awiddersheim [email protected] wrote:

Anyway this can get merged into master soon? This is getting increasingly harder to merge as time goes on.

—
Reply to this email directly or view it on GitHub.

awiddersheim · 2014-10-19T19:07:56Z

Do you want to rebase onto master and merge or shall I?

jrossi · 2014-10-19T19:08:46Z

Just merge / rebate I will accept once it's clean.

On Oct 19, 2014, at 3:08 PM, awiddersheim [email protected] wrote:

Do you want to rebase onto master and merge or shall I?

—
Reply to this email directly or view it on GitHub.

awiddersheim · 2014-10-19T19:11:18Z

I am not even sure the best way to do that at this point.

jrossi · 2014-10-19T19:15:53Z

No problem I will do it from home.

awiddersheim · 2014-10-19T19:17:08Z

Actually, I will git it a shot tomorrow morning and let you know if I need help if things go south.

jrossi · 2014-10-19T19:20:43Z

Example of how if you want to play:

# git checkout master 
# git pull https://github.com/ossec/ossec-hids master
# git checkout fix-manage-client-keys
# git merge master 
# fix errors 
# git add files-fixed
# git commit -m "merge master" 
# git push origin fix-mange-client-keys"

Conflicts: src/InstallAgent.sh.old src/addagent/manage_keys.c

Fix manage agents keys

awiddersheim added 6 commits October 9, 2014 10:08

Fix error messages in manage_agents

d973837

Add error checking to chmod() in manage_agents

bc3058b

Add tmp directory creation to InstallAgent.sh

1ee0c00

Added code to make temporary file names consistant

a51584e

This code should make the temporary file names a bit more consistant with what gets set in the global constants. This way if those ever change the temporary files should as well and hopefully continue to make sense as to what they do.

Add headers for Windows builds

b2f184b

This lets the user see progress and where errors/warnings are happening a bit easier. It also is similar to how the output done by the Make stuff on the NIX side.

awiddersheim force-pushed the fix_manage_agents_keys branch from bcb81b4 to b2f184b Compare October 9, 2014 14:10

Merge branch 'master' into fix_manage_agents_keys

6851f87

Conflicts: src/InstallAgent.sh.old src/addagent/manage_keys.c

jrossi added win32 rn:agent rn:install rn:important enhancement labels Oct 20, 2014

jrossi added this to the ossec-hids-2.9 milestone Oct 20, 2014

jrossi added the bug label Oct 20, 2014

jrossi added a commit that referenced this pull request Oct 20, 2014

Merge pull request #336 from awiddersheim/fix_manage_agents_keys

9497895

Fix manage agents keys

jrossi merged commit 9497895 into ossec:master Oct 20, 2014

awiddersheim deleted the fix_manage_agents_keys branch October 20, 2014 02:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix manage agents keys #336

Fix manage agents keys #336

awiddersheim commented Oct 6, 2014

awiddersheim commented Oct 6, 2014

jrossi commented Oct 6, 2014

awiddersheim commented Oct 6, 2014

awiddersheim commented Oct 6, 2014

awiddersheim commented Oct 6, 2014

awiddersheim commented Oct 7, 2014

jrossi commented Oct 7, 2014

awiddersheim commented Oct 7, 2014

jrossi commented Oct 7, 2014

jrossi commented Oct 9, 2014

awiddersheim commented Oct 9, 2014

jrossi commented Oct 9, 2014

awiddersheim commented Oct 9, 2014

cgzones commented Oct 10, 2014

awiddersheim commented Oct 11, 2014

awiddersheim commented Oct 19, 2014

jrossi commented Oct 19, 2014

awiddersheim commented Oct 19, 2014

jrossi commented Oct 19, 2014

awiddersheim commented Oct 19, 2014

jrossi commented Oct 19, 2014

awiddersheim commented Oct 19, 2014

jrossi commented Oct 19, 2014

Fix manage agents keys #336

Fix manage agents keys #336

Conversation

awiddersheim commented Oct 6, 2014

awiddersheim commented Oct 6, 2014

jrossi commented Oct 6, 2014

awiddersheim commented Oct 6, 2014

awiddersheim commented Oct 6, 2014

awiddersheim commented Oct 6, 2014

awiddersheim commented Oct 7, 2014

jrossi commented Oct 7, 2014

awiddersheim commented Oct 7, 2014

jrossi commented Oct 7, 2014

jrossi commented Oct 9, 2014

awiddersheim commented Oct 9, 2014

jrossi commented Oct 9, 2014

awiddersheim commented Oct 9, 2014

cgzones commented Oct 10, 2014

awiddersheim commented Oct 11, 2014

awiddersheim commented Oct 19, 2014

jrossi commented Oct 19, 2014

awiddersheim commented Oct 19, 2014

jrossi commented Oct 19, 2014

awiddersheim commented Oct 19, 2014

jrossi commented Oct 19, 2014

awiddersheim commented Oct 19, 2014

jrossi commented Oct 19, 2014