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

Fix overwriting an agent counter with sender counter during updating keys #2064

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

staskysel
Copy link

Hi. I'm a newbie here, so please forgive me, if I fail to follow some kind of "code of conduct".

In send_msg(), CreateSecMSG() uses (for persisting sender counter) keys structure, which can be modified by main thread calling OS_UpdateKeys(). Thus main thread can interfere with manager thread calling CreateSecMSG().

To fix this, CreateSecMSG() must be called after obtaining sendmsg_mutex lock.

Otherwise under certain circumstances, quite common when adding new agents, an existing agent's counter is overwritten by sender counter e.g. in the following scenario:

Main thread (HandleSecure()):

  1. Receives a message from an initialized (installed) agent, that is not configured in OSSEC server (yet)
  2. Calls check_keyupdate()
  3. If keys file was modified, calls OS_UpdateKeys()
  4. Calls OS_FreeKeys()
  5. Sets keys.keysize = 0
  6. Waits for 1 second with sleep(1), thus transferring control to the main thread and increasing probability of the following.

Manager thread:

  1. Calls CreateSecMSG() (e.g. from send_msg() called from send_file_toagent() called from read_controlmsg() called from wait_for_msgs())
  2. Calls StoreSenderCounter(), which stores sender counter to keys->keyentries[keys->keysize]->fp
  3. Since keys->keysize is 0, instead of writing to "queue/rids/sender_counter" file, the counter is written to "queue/rids/NNN", where NNN is the lowest agent ID.
  4. This effectively disconnects the agent with the lowest ID, as new messages from it are not accepted, because counter doesn't match.

@atomicturtle
Copy link
Member

Looks good to me, Im running test to try and duplicate the scenario you described right now so I can get a before and after verification. Sorry about the delay!

@atomicturtle
Copy link
Member

Looks good here, and you're right I had to get something like 120 agents registering at once to hit this one

@atomicturtle atomicturtle merged commit d42ae4a into ossec:master Aug 7, 2023
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.

2 participants